Skip to content
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

Replace Google Closure minifier with Terser #90

Merged
merged 7 commits into from
May 11, 2020

Conversation

j-maas
Copy link
Contributor

@j-maas j-maas commented Mar 20, 2020

Fixes #83. Replaces problematic Google Closure Compiler with terser.

We cannot use uglifyjs, because the main.js contains modern JavaScript (const) whereas uglifyjs can only deal with ES5. uglify-es is abandoned, but terser is a really nice replacement.

@dillonkearns
Copy link
Owner

Thanks for doing the research and making a PR for this! 🙏

A couple of thoughts:

  1. Does this need to be run with two passes?

See this note from this page: https://guide.elm-lang.org/optimization/asset_size.html

Note 1: uglifyjs is called twice there. First to --compress and second to --mangle. This is necessary! Otherwise uglifyjs will ignore our pure_funcs flag.

  1. Do we need to avoid running this on JS files with these configs? Since the JS files aren't guaranteed to have pure functions, we need to be sure we're not breaking any JS code in the user's project.

@j-maas
Copy link
Contributor Author

j-maas commented Mar 21, 2020

  1. Does this need to be run with two passes?

No. I tested it running in two passes and it produces the same output as with one pass.

  1. Do we need to avoid running this on JS files with these configs? Since the JS files aren't guaranteed to have pure functions, we need to be sure we're not breaking any JS code in the user's project.

I guess in general, yes. However, even without the Elm-specific config, it does a good job, so we could run it with the default config until we find a way to differentiate:

  • 691.340 bytes raw
  • 160.368 bytes with default config
  • 149.607 bytes with Elm-specific config

@dillonkearns
Copy link
Owner

I did some digging, and was just about to give up, but I finally found this option! Looks like it's quite easy to only apply the plugin to files based on file extension:

module.exports = {
  optimization: {
    minimize: true,
    minimizer: [
      new TerserPlugin({
        test: /\.js(\?.*)?$/i,
      }),
    ],
  },
};

https://github.com/webpack-contrib/terser-webpack-plugin#test

I know that for many cases, applying these optimizations won't cause problems for their JS code. But since it's a framework that supports many different use cases, I want to be conservative about ensuring correctness here. Are you up for trying this test option out to see if it successfully applies that Terser config only to Elm files? Then maybe we could have two different instances of Terser, one that only applies to .elm files, and one that applies to anything that's not .elm?

Something like this:

module.exports = {
  optimization: {
    minimize: true,
    minimizer: [
      new TerserPlugin({
        test: /\.elm$/i,
      }),
  new TerserPlugin({
        test: /(?!\.elm$)/i,
      }),
    ],
  },
};

Note: I didn't test that regex, I just tried this trick for negating: https://stackoverflow.com/questions/1538512/how-can-i-invert-a-regular-expression-in-javascript.

But the really important thing is testing if we can only run those Elm-specific options on Elm files, so let's see how that experiment goes first.

Let me know what you think! I'm happy to help as well. Feel free to ping me on Slack to coordinate.

Parallel flag

Also, I'm up for merging and releasing this once we get the correctness piece. But for performance, it seems like we could use the parallel option here, much like we discussed in the context of UglifyJS: https://github.com/webpack-contrib/terser-webpack-plugin#parallel.

Y0hy0h added 2 commits March 23, 2020 13:26
For the time being, we cannot guarantee that the file to be compressed is generated by Elm. Therefore we cannot apply its special optimizations.
@j-maas
Copy link
Contributor Author

j-maas commented Mar 23, 2020

I've run into issues with the approach you suggested: Webpack does not look at the original file name, but considers the generated JavaScript file. I.e., the .elm extension does not match the files we are interested in. I tested this by adding test: /\.elm$/i and the resulting main.js was really big, which indicates that the minifier wasn't run.

I think at the point where the test is done, the file is a JavaScript file. So either we need to find a way to detect what the source file was, or add a special extension ourselves.

For the time being, I've disabled those Elm-specific optimizations such that there are no problems with other .js files. (I'm unsure how to test this; how do I include a .js file in the final build? Similar to the .css by including it from index.js?)

@dillonkearns
Copy link
Owner

dillonkearns commented May 11, 2020

Hi @y0hy0h, thanks so much for your patience on this!

Since create-elm-app uses a similar Terser config:

https://github.com/halfzebra/create-elm-app/blob/ced4a9107359cdf3c2da4a108bd37e6446c715a0/config/webpack.config.prod.js#L60-L115

I think it's reasonable to assume that the setup (including the Elm optimizations) is safe. The functions to optimize:

"F2", "F3", "F4", "F5", "F6", "F7", "F8", "F9", "A2", "A3", "A4", "A5", "A6", "A7", "A8", "A9"

Is safe because those are all functions that Elm controls and can safely mark as pure. I'll merge this in for this release.

I would love to hear how it goes for you with your Windows set up! It would be great if it got elm-pages build working on your machine 👍

Thanks again for the PR!

@dillonkearns dillonkearns merged commit f481a6b into dillonkearns:master May 11, 2020
@j-maas j-maas deleted the terser branch May 12, 2020 08:38
@j-maas
Copy link
Contributor Author

j-maas commented May 12, 2020

@dillonkearns The freshly cloned elm-pages-starter repo works flawlessly on my machine! Thanks for all the work put in!

Unfortunately my own project doesn't compile due to internals in Pages.elm not typechecking and Elm missing in compile-elm.js. But I'm not currently working on it, so I won't dig into what the difference is to the starter repo right now. When I figure out where the problem is, I might open an issue, but for now let's assume that it works, as the starter repo proves. :)

@dillonkearns
Copy link
Owner

Thank you for the update @y0hy0h!

Did you update both the NPM and Elm package for your own project?

@j-maas
Copy link
Contributor Author

j-maas commented May 12, 2020

@dillonkearns Haha, yes that was it actually! 🤦 Now I'll have to migrate to the new version, as I used Pages.Document, but it's all nicely documented in the changelog. 👍

@dillonkearns
Copy link
Owner

Oh good, glad it was an easy fix! Yeah, the Pages.Document change is actually quite straightforward, you just remove the function call and leave the record in there directly. Funny how it's hard to see those designs early on!

@j-maas
Copy link
Contributor Author

j-maas commented May 12, 2020

Yeah, already fixed. :) (I've decided to spend some time on this project after all... :D)

elm-pages was already very pleasant, and it just keeps getting nicer and nicer!

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.

elm-pages build crashes on Windows due to invalid path
2 participants