-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add note about --optimize-minimize is not required for tree shaking #1474
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
Conversation
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.
content/guides/tree-shaking.md
Outdated
@@ -84,3 +84,5 @@ function(e,t,n){"use strict";function r(e){return e*e*e}t.a=r} | |||
/* ... */ | |||
function(e,t,n){"use strict";Object.defineProperty(t,"__esModule",{value:!0});var r=n(0);console.log(n.i(r.a)(5))} | |||
``` | |||
|
|||
T> Note, using the `--optimize-minimize` flag is NOT required for tree shaking to occur. Tree shaking will occur when using a production configuration. This can also be configured directly in a `webpack.config.js` file. Read [production build](/guides/production) for an in-depth guide on how to create a production config file. |
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.
Instead of saying:
Tree shaking will occur when using a production configuration.
I think we should be more specific on exactly what's necessary which I think, as I mentioned in #1473, is allowing webpack to actually resolve import
/ export
statements (i.e. disabling any transpilers like babel from resolving them). And, including the UglifyJsPlugin
. The import
/ export
is already mentioned at the top of this guide so maybe just noting that either --optimize-minimize
should be used (which includes the UglifyJsPlugin
under the hood) or the plugin can be configured directly (likely in a "production" configuration).
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.
Okay, I think that makes sense. However, I am unsure about the first part. Do you mean using something like babel-plugin-resolver when you say resolving using transpilers? I am not sure how transpilers work, but I am assuming loading your JS files with babel-loader
does not resolve any imports, right?
What do you think about something along the lines of
Note, using the
--optimize-minimize
flag is NOT required for tree shaking to occur. Tree shaking will occur when Webpack resolves your import/export statements and when you are including theUglifyJsPlugin
. You can either include theUglifyJsPlugin
using the--optimize-minimize
flag or by adding the plugin manually to yourplugins
key of your configuration file. Read production build for an in-depth guide on how to create a production config file.
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.
However, I am unsure about the first part. Do you mean using something like babel-plugin-resolver when you say resolving using transpilers? I am not sure how transpilers work, but I am assuming loading your JS files with babel-loader does not resolve any imports, right?
Depends on how you have the babel
and the babel-loader
configured. With certain plugins, you'll need to set { modules: false }
in order to let webpack resolve the import
and export
syntax.
What do you think about something along the lines of...
Definitely sounds better... I might reword/simplify as:
Note that the
--optimize-minimize
flag enables tree shaking by including theUglifyJsPlugin
behind the scenes. Alternatively, theUglifyJsPlugin
can be included manually in theplugins
section of your configuration file. The plugin, combined with webpack's resolving ofimport
andexport
statements, is what makes tree shaking possible. See the production build guide for more information.
What do you think?
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.
Yes, this is a better wording, I agree. I have updated the diff to your suggestion.
Great, thanks @esbenp! Re-running the build due to some timeouts (which we should really be ignoring -- will address this finally in a separate PR). Once the build passes we should be able to get this merged -- at which point I'll close out #1473 but make a note in #1331 that this should be one of the things we address in the rewrite. |
Idea for #1473 (also see #1331). Opener for discussion - feedback welcome.