Skip to content

Minify prelude #761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Minify prelude #761

wants to merge 3 commits into from

Conversation

dave
Copy link
Contributor

@dave dave commented Feb 25, 2018

This PR adds minified versions of the prelude JS. This saves 8910 bytes on minified output. We can compare the size of the outputted JS for a trivial hello world:

package main

func main() {
  println("Hello, World!")
}

gopherjs build -m main.go

Master: 51170 bytes
This PR: 42260 bytes

@dave
Copy link
Contributor Author

dave commented Feb 25, 2018

I did the minifying with https://skalman.github.io/UglifyJS-online/ - we need to remember to update the minified versions whenever updating the un-minified prelude! @shurcooL

@myitcv
Copy link
Member

myitcv commented Feb 26, 2018

@dave 💯 👍 to what you're proposing here. Would love to benefit from this too.

we need to remember to update the minified versions whenever updating the un-minified prelude

I think we can automate this fairly easily and add a check to circle.yml to ensure the committed minified code is consistent with the unminified code.

Something along the lines of:

  • add a package.json to the repo root, have uglify-es as a dev dependency
  • add a // +build ignore-d main-package .go file to the compiler/prelude directory that outputs github.com/gopherjs/gopherjs/compiler/prelude.Prelude which can then be piped to uglifyjs and then written to a generated file that defines github.com/gopherjs/gopherjs/compiler/prelude.PredudeMinified in another file in compiler/prelude
  • add a step to circle.yml to re-run this generation step via go run, and then check that git status --porcelain is the empty string (else fail the build)

Makes things a little less fragile.

Happy to help flesh this out further if it sounds reasonable?

Copy link
Member

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #762 (comment).

If you still want to do this, I'm okay with it. The prelude doesn't change very often, so I'm okay with keeping it manual for now. We can automate later if needed—it's not worth all the extra machinery overhead now.

Just make the following changes. Only add Minified add and inline its contents.

Document it:

// Minified is Prelude ran through UglifyJS 3 via https://skalman.github.io/UglifyJS-online/.
const Minified = `...`

} else {
preludeString = prelude.Prelude
}
if _, err := w.Write([]byte(preludeString)); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use io.WriteString here.

@dave
Copy link
Contributor Author

dave commented Feb 27, 2018

Done!

@myitcv
Copy link
Member

myitcv commented Feb 27, 2018

We can automate later if needed—it's not worth all the extra machinery overhead now.

I don't think the extra machinery creates that much overhead; I just fleshed out what I outlined above:

#765

And the benefit is that it saves having to manually re-run minification at all.

No need to minify parts of prelude separately. It actually improves minification slightly when done as a whole (32895 bytes -> 32763 bytes).

Adjust style of if statement to make it simpler/shorter and rename preludeString variable to be more descriptive. It's prelude JavaScript code.
Copy link
Member

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'm quite confident about merging this change because we run all tests with minification in CI (gopherjs test --minify) and they're passing. Still, just in case, I'll bump up the GopherJS version so we can distinguish this from previous version if there are any potential problems.

Thanks @dave and @myitcv!

@dmitshur dmitshur closed this in 9c69860 Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants