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

Switch to terser-webpack-plugin #1158

Merged
merged 1 commit into from Oct 10, 2018
Merged

Switch to terser-webpack-plugin #1158

merged 1 commit into from Oct 10, 2018

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Oct 9, 2018

Since the webpack 4 default of uglifyjs-webpack-plugin uses uglify-es, which has been deprecated in favour of terser. webpack 5 is due to use terser-webpack-plugin, at which point we can remove this.

terser includes multiple correctness and performance fixes not present in uglify-es.

Fixes #1146.

@edmorley edmorley added this to the v9 milestone Oct 9, 2018
@edmorley edmorley self-assigned this Oct 9, 2018
@edmorley edmorley requested a review from eliperelman Oct 9, 2018
or even use a different minifier, override `optimization.minimizer`.

Note: If switching to [babel-minify-webpack-plugin](https://github.com/webpack-contrib/babel-minify-webpack-plugin)
Copy link
Member Author

@edmorley edmorley Oct 9, 2018

Choose a reason for hiding this comment

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

I've intentionally removed this reference to babel-minify-webpack-plugin, since it's still virtually unmaintained/buggy/slow. If that changes in the future we can re-add.

Copy link
Member

@eliperelman eliperelman left a comment

LGTM!

@edmorley
Copy link
Member Author

@edmorley edmorley commented Oct 9, 2018

I was thinking about this PR some more today. Since we manually configure terser-webpack-plugin, we have to choose whether to set sourceMaps to true/false. This is fine if people use the in-built web preset devtool option, however for Neutrino 8 upgraders it's likely they may have:

module.exports = {
  use: [
    ['@neutrinojs/react', {
      // ...
    }],
    (neutrino) => {
      if (process.env.NODE_ENV === 'production') {
        neutrino.config.devtool('source-map');
      }
    },
  ]
};

In this case, we'll pass sourceMaps: false to terser-webpack-plugin.

The only thing I think we can do, is to perhaps add a warning (or throw) in the webpack() handler, like we do for the vendor entrypoint check, which checks whether devtool and sourceMap are out of sync (if the terser plugin found).

@eliperelman, thoughts?

@eliperelman
Copy link
Member

@eliperelman eliperelman commented Oct 9, 2018

@edmorley I think that solution will work, but we should probably use a warning since it may be desired by the user to have them out of sync for some reason.

Since the webpack 4 default of `uglifyjs-webpack-plugin` uses `uglify-es`,
which has been deprecated in favour of `terser`. webpack 5 is due to
use `terser-webpack-plugin`, at which point we can remove this.

`terser` includes multiple correctness and performance fixes not
present in `uglify-es`.

Fixes #1146.
@edmorley
Copy link
Member Author

@edmorley edmorley commented Oct 10, 2018

I've updated the PR:

  • rebased on master's webpack-chain v5 upgrade
  • added a check for misconfiguration in handlers.js (see comment there for why throwing rather than a console.error())
  • added migration guide changes

@edmorley edmorley requested a review from eliperelman Oct 10, 2018
@edmorley edmorley merged commit 3e5839b into neutrinojs:master Oct 10, 2018
2 checks passed
@edmorley edmorley deleted the terser-webpack-plugin branch Oct 10, 2018
edmorley added a commit that referenced this issue Nov 20, 2018
Since as of webpack 4.26.0 `terser-webpack-plugin` is the new default,
so we no longer need to set it manually:
https://github.com/webpack/webpack/releases/tag/v4.26.0

This is a partial revert of #1158.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants