-
Notifications
You must be signed in to change notification settings - Fork 570
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
Minify prelude #761
Conversation
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 |
@dave 💯 👍 to what you're proposing here. Would love to benefit from this too.
I think we can automate this fairly easily and add a check to Something along the lines of:
Makes things a little less fragile. Happy to help flesh this out further if it sounds reasonable? |
There was a problem hiding this 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 = `...`
compiler/compiler.go
Outdated
} else { | ||
preludeString = prelude.Prelude | ||
} | ||
if _, err := w.Write([]byte(preludeString)); err != nil { |
There was a problem hiding this comment.
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.
Done! |
I don't think the extra machinery creates that much overhead; I just fleshed out what I outlined above: 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.
There was a problem hiding this 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.
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:
gopherjs build -m main.go
Master: 51170 bytes
This PR: 42260 bytes