-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update webpack to v5.x #2263
Update webpack to v5.x #2263
Conversation
Great work. I'll have a closer look next week. Are there more differences like the one I commented about in #2263 (comment)? I know that |
Other than |
@walbo, any chances to bring this PR up to date? I'd love to land that before updating WordPress packages in core for the upcoming WordPress 6.0 release. |
@gziolo Yes, I can do that. Will try to make it happen later today, if not I'll to it tomorrow. |
This PR will unblock #2519 which brings React Fast Refresh support to WordPress core for block development with |
'lodash-es': 'lodash', | ||
}, | ||
}, | ||
stats: 'errors-only', |
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.
Not sure why v5 shows alot more stats, but changed it to only show errors. From the logs I only see v4 logging an hash and the webpack version.
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.
Should we include also warnings: 'errors-warnings'
? Unless it's too noisy. We use the default value in Gutenberg.
moduleIds: mode === 'production' ? 'deterministic' : 'named', | ||
minimizer: [ | ||
new TerserPlugin( { | ||
extractComments: false, |
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.
To keep the License comments inside the minimized files we need to set this.
Omitting this will create a .min.LICENCE.txt
file that contains the licenses. See https://webpack.js.org/plugins/terser-webpack-plugin/#extractcomments
Would be great to enable this to make the minimized files smaller, but since they are inlined today I opted to still have them inline.
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.
This is what we have in Gutenberg:
I think the part for translation comments was necessary after migration to v5.
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.
Related PRs: WordPress/gutenberg#22990 and WordPress/gutenberg#28231. @swissspidy, do we need the same handling in WordPress core? We have non-minified files included, too.
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.
extractComments: false
makes sense.
Using the same config as Gutenberg to keep translator comments and avoid mangling i18n functions also makes sense
@gziolo The PR should be up to date. Added a couple of comments. |
We can use the PR with webpack 5 upgrade in Gutenberg as a reference: WordPress/gutenberg#33818 |
minimize: false, | ||
moduleIds: 'deterministic', | ||
}; | ||
} else if ( mode !== 'production' ) { |
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.
Nice refactoring here 👍🏻
moduleIds: mode === 'production' ? 'deterministic' : 'named', | ||
minimizer: [ | ||
new TerserPlugin( { | ||
extractComments: false, |
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.
This is what we have in Gutenberg:
I think the part for translation comments was necessary after migration to v5.
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.
Code wise it looks good. It simply needs more testing to ensure we don't break any existing workflows. I would appreciate some sanity check from someone else.
I noticed that this PR is included in the WordPress 6.0 Project board. |
Yes, it's correctly included. I asked the Build/Test Tools component maintainers to run additional checks with the intention to land it before WordPress 6.0 Beta 1. |
Thanks for the status update, Greg! |
I'm preparing the final patch to land in WordPress core so I opened #2561 to ensure that CI checks remain green after rebasing and resolving conflicts with |
Committed with https://core.trac.wordpress.org/changeset/53135. Major props to @walbo for making it happen! 🎉 |
Trac ticket: https://core.trac.wordpress.org/ticket/51750
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.