Minified was 32740 bytes before.

Now it's 41046 bytes. How come? Can we make it match previous output (from https://skalman.github.io/UglifyJS-online/ with its default options) or at least be closer to it in size?

Thanks, good catch. So it appears Actually it appears https://skalman.github.io/UglifyJS-online/ is using some non-default options. I've added uglify_options.json in a new commit and now things look better:

$ git rev-parse HEAD
5633de29373c2e6456ca8c74313b09785d39524f
$ cat compiler/prelude/prelude_min.go | wc -c
33300

Is out guaranteed not to contain ` characters?

If so, please document the reason. Otherwise, consider using strconv.Quote or something equivalent that is safe.

Also, Minified should have a comment. It's an exported symbol.

Is out guaranteed not to contain ``` characters?

Good catch. I don't know, but I've now made it safe in the latest commit (we need to strings.Replace, not quote). In any case the worst that would have happened here is that we get a compile error. But that's a moot point because we now output the safe version.

Also, Minified should have a comment. It's an exported symbol.

Done. And I've also commented Prelude because that wasn't commented.

Calling os.Exit(1) after log.Fatalf is a no-op, it will never run.

Can you refactor the code to follow this pattern?

package main

import (
	"log"
)

func main() {
	err := run()
	if err != nil {
		log.Fatalln(err)
	}
}

func run() error {
	// move the code that can fail here
	if err != nil {
		return err
	}
	// ...
	return nil
}

Done

Also, I would suggest renaming regen.go to gen.go to be more consistent with other Go projects (gen.go generates prelude_min.go, "regenerates" is not a better description). Or genmin.go if you want it to be more specific.

Examples:

• https://github.com/golang/go/blob/master/src/crypto/md5/gen.go
• https://github.com/golang/go/blob/master/src/sort/genzfunc.go
• https://github.com/golang/go/blob/master/src/time/genzabbrs.go

Done.

I'm not familiar with npm, but why npm install is required here? (comments would be helpful)

npm install (distinct from the previous step) with no arguments references the nearest (in directory hierarchy terms, looking upwards) package.json file. This file specifies the required npm (dev) dependencies; in this case uglify-es. I can add a comment to the effect the repo root package.json is the implicit reference here.

IIUC, the last period is not needed:

golang/go#13560 (comment)

Fixed.

I think this time is ok (I think @shurcooL has already confirmed), but I'd separate this PR into moving this part and generating this part so that we could confirm the generated code was same.

Oh, #791 is the moving part? I missed that, sorry.

Yes #791 is separate. Unless we want to collapse this and #791 into one PR. Which makes sense from my perspective.

I think it is fragile to assume there is no stderr when err is not found. Isn't just Output enough?

In normal operation uglify-es (and other programs) output nothing to stderr. But in an error scenario many (if not most) output to stderr. So it's best to capture both here for the scenario where we do error so that we can output as useful an error message as possible. Or did I misunderstand the point you were making?

strconv.Quote(string(out)) or fmt.Sprintf("%q", string(out)) would be better instead of safeOut (I'm not sure safeOut is really enough escaping)

safeOut is I think enough escaping because the only thing that can't appear between back quotes is a backquote. But I agree using strconv.Quote makes the code cleaner (although I note that the string() conversion of the []byte variable is unnecessary when using %q)

npm install (distinct from the previous step) with no arguments references the nearest (in directory hierarchy terms, looking upwards) package.json file. This file specifies the required npm (dev) dependencies; in this case uglify-es. I can add a comment to the effect the repo root package.json is the implicit reference here.

In normal operation uglify-es (and other programs) output nothing to stderr. But in an error scenario many (if not most) output to stderr. So it's best to capture both here for the scenario where we do error so that we can output as useful an error message as possible. Or did I misunderstand the point you were making?

safeOut is I think enough escaping because the only thing that can't appear between back quotes is a backquote. But I agree using strconv.Quote makes the code cleaner (although I note that the string() conversion of the []byte variable is unnecessary when using %q)

Yes #791 is separate. Unless we want to collapse this and #791 into one PR. Which makes sense from my perspective.

Fixed.

(I thought I've already mentioned but just in case)

Could we have another PR to move this part so that we could confirm that the generated min codes are same?

Done. See #794

#791 now just reflects the move to separate .js files; as you can see from the diff the minified prelude does not change with respect to #784.

It does change in #794 post the prettier change because prettier does the equivalent of a gofmt on our js code.

I'm confused: #794 regenerates and moves the minimized js?

@hajimehoshi I've updated the issue description in #793 to explain how the PRs are linked.

OK so does #794 do moving the minimized code and reformatting the original js at the same time?

Correct

Yeah, so my point is:

Could we have another PR to move this part so that we could confirm that the generated min codes are same?

In #784, it is hard to say the generated codes are same, right?

We're going to get a diff introduced by switching to use uglify-es at some point, simply because it's a different program/environment minifying the code with nothing that guarantees the same behaviour. So I'm not entirely convinced of the benefit of having a separate PR that moves the line of minified code from prelude.go to prelude_min.go.

To my mind the things that matter here are:

1. the tests all pass (they intentionally run with minified code)
2. the size of the minified prelude is roughly the same:

$ git rev-parse HEAD
e14987c0ef06db387b90fec85e8d06dc05598e24
$ goexec -quiet 'io.WriteString(os.Stdout, prelude.Minified)' | wc -c
32740
$ git checkout regen_min
$ goexec -quiet 'io.WriteString(os.Stdout, prelude.Minified)' | wc -c
32767

Does that sound a reasonable compromise to you?

I've just pushed up another commit that ensures we follow the exact same convention on comments for what it's worth.

I think this is okay.

Yeah, fair enough. Thank you!

Please make this match the standard way of marking generated files, as defined at https://golang.org/s/generatedcode.

I suggest:

// Code generated by genmin; DO NOT EDIT.

The missing period at the end is a mandatory change. Changing the period after genmin is optional, but more common and consistent with:

Thanks - we'd been back and forward on this already, but both @hajimehoshi had a link to the wrong version of Rob's proposal. Will fix.

Oh I was misunderstanding that golang/go#13560 (comment) was the final suggestion, without period...

I think this is okay.

bpkg.Dir is not a very readable name here. I would've copied the importPathToDir helper from:

Then code above can be gopherjsRoot := importPathToDir("github.com/gopherjs/gopherjs"), and this line can be filepath.Join(gopherjsRoot, "node_modules", ".bin", "uglifyjs").

In theory, you could also just resolve both dirs directly via importPathToDir("github.com/gopherjs/gopherjs/compiler/prelude") and importPathToDir("github.com/gopherjs/gopherjs/node_modules/.bin"). importPathToDir can resolve any existing directory in a GOPATH workspace, it doesn't need to be a valid Go package.

Not a big deal, just pointing it out.

I'd tend to disagree; using go/build a lot, it feels very idiomatic to do exactly this sort of resolution. Wrapping exactly the same behaviour in a function just adds to the non-exported API surface and therefore cognitive overhead to my mind (i.e. I have to remember that importPathToDir exists and to then use it). So I'll leave as is if that's ok with you.

Comments should have sentences that end with a period, to follow Go style (of having proper grammar).

Also, I'm not sure how accurate "behind the github.com/gopherjs/gopherjs/js package" statement is. The prelude contains things needed for all of GopherJS generated code to work, not just js package. I would remove it (since it's not very accurate).

Will be fixed in my next commit.

Same here, sentences should end in periods. This applies to all Go code we write in the GopherJS project.

Ditto.

This isn't a big deal, but it's pretty unusual to see a []byte variable named byts. As far as I understand, it's a shortened version of "bytes"? It comes up a total of 5 times in the Go project.

In this context, just b or out would be more idiomatic and clear I think.

I see that out is already taken below, in that case, I would make this variable name more specific. For example, minified.

The reason I point this out is because I think it's a good idea to strive to write Go code that looks like Go code other Go programmers are most likely to write.

Fixed.