-
-
Notifications
You must be signed in to change notification settings - Fork 201
Update Webpack to v5 (+ other dependencies) #645
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
I fixed the failing Babel test which wasn't really related to Babel but to the new This option tells Webpack which ES version it should use for the code it generates (for instance in JSONP callbacks) which is not transpiled by Babel. By default it is set to Since some users may not want that behavior (IE11 does not support arrow functions) I changed the default value in Encore to 5. Regarding the deprecation messages I confirm that they are all triggered by plugins (and maybe by the Webpack core itself for some of them):
Not really related but I also removed our old deprecation messages since they have all been there for a while now. |
It seems that the hack we are using for The tests are currently failing because of the following change:
Since we are directly checking if the chunk name matches Sadly, after fixing that part we get the following error message:
Which is exactly how webpack-encore/lib/config-generator.js Lines 517 to 527 in 5640271
|
Is there still a legitimate reason to use/have |
@Kocal I'd say that it's useful for people that are not using the Encore Bundle since they can't predict the names of the shared chunks Webpack will generate for them when using |
* @param {function} pluginOptionsCallback | ||
* @returns {Encore} | ||
*/ | ||
configureMiniCssExtractPlugin(loaderOptionsCallback, pluginOptionsCallback = () => {}) { |
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 this be bundled in the webpack 5 upgrade, or be submitted separately ?
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.
I originally added it into #564 in case people wanted to disable the new hmr
option (which is also included in this one). So, maybe we could ship it separately but it'd be preferable if it was done before the mini-css-extract-plugin
upgrade.
@@ -546,7 +546,7 @@ describe('WebpackConfig object', () => { | |||
|
|||
it('Calling with "includeNodeModules" option', () => { | |||
const config = createConfig(); | |||
config.configureBabel(() => {}, { include_node_modules: ['foo', 'bar'] }); | |||
config.configureBabel(() => {}, { includeNodeModules: ['foo', 'bar'] }); |
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 already be done in the existing 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.
I remember keeping the include_node_modules
check on purpose since we only deprecated that version. But maybe we should have had an additional check for the uppercase version.
@@ -39,6 +39,7 @@ describe('bin/encore.js', function() { | |||
` | |||
const Encore = require('../../index.js'); | |||
Encore | |||
.enableSingleRuntimeChunk() |
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.
shouldn't this be done in the existing version already ?
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.
We could but that's not a big issue since the current version allows not to call enableSingleRuntimeChunk()
/disableSingleRuntimeChunk()
(unless you also called createSharedEntry()
it defaults to the disableSingleRuntimeChunk()
behavior with deprecation messages).
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.
I mean, we should avoid using deprecated API in our testsuite, unless we are specifically testing the BC layer (in which case the test gets removed when dropping it, or changed to a test asserting an exception).
That's the policy we have in Symfony itself (we even make the testsuite fail if deprecations are triggered in non-legacy tests).
About A) Remove B) Replace it with some interface to make it easier/obvious to the user to add their own cache group - e.g. to get some end-config that looks similar to this: https://webpack.js.org/plugins/split-chunks-plugin/#split-chunks-example-3 It could be something like:
That would be the lowest-level way to handle this. We might choose to instead/also add something more user-friendly:
Not sure how far we need to go. If we did the first and added good documentation with a "vendors" example, that will go a long way. |
Updated the PR with
|
…createSharedEntry() (Lyrkan) This PR was squashed before being merged into the master branch (closes #680). Discussion ---------- Add Encore.addCacheGroup() method and depreciate Encore.createSharedEntry() As discussed in #645 (comment) our `Encore.createSharedEntry()` hack won't work anymore with Webpack 5. Since it was mostly something that was added to make the transition from Webpack 3 to Webpack 4 less painful it's probably time to depreciate it and encourage people to use cache groups properly instead. This PR adds a new `Encore.addCacheGroup()` method that, as its name implies, simply adds a new cache group to the config: ```js Encore.addCacheGroup('vendor', { test: /[\\/]node_modules[\\/]react/ }); ``` To make it a bit easier in case people want to directly reference things from the `node_modules` directory I also added a `node_modules` option which is basically a shorthand that sets the `test` option: ```js Encore.addCacheGroup('vendor', { node_modules: ['react', 'react-dom'] }); ``` Commits ------- 7e32b0b Add a warning when calling addCacheGroup() with 'vendors' or the same than in createSharedEntry() e4095b3 Add Encore.addCacheGroup() method and depreciate Encore.createSharedEntry()
37e9ea3
to
beb34ac
Compare
PR updated:
There are some new failing tests, but they are all caused by the same thing and should be fixed in the next release of Webpack (see webpack/webpack#10661). |
Awesome @Lyrkan! Thanks for keeping this up to date - we should be able to get Encore released fast once Webpack gets here - whenever that is :). |
f8b84f9
to
4576fc3
Compare
const isVue2 = getVueVersion(config) === 2; | ||
config.addCacheGroup('vuejs', { | ||
test: isVue2 ? | ||
/[\\/]node_modules[\\/]vue[\\/]/ : | ||
/[\\/]node_modules[\\/]@vue[\\/]/ | ||
}); |
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.
Quick explanation about why this is needed.
This is actually not directly related to Webpack 5 but because of how vue
is now splitted into multiple packages under the @vue
scope.
The imported module when you import vue
does not do much:
import { warn } from '@vue/runtime-dom';
export * from '@vue/runtime-dom';
// This entry exports the runtime only, and is built as
const compile = () => {
if ((process.env.NODE_ENV !== 'production')) {
warn(`Runtime compilation is not supported in this build of Vue.` +
( ` Configure your bundler to alias "vue" to "vue/dist/vue.esm-bundler.js".`
) /* should not happen */);
}
};
export { compile };
With Webpack 4 the cache group seemed to worked because it created a shared file with the code of the compile
function and the declaration of the multiple exports from @vue/runtime-dom
(without their actual code... so not really useful).
With Webpack 5 I'm guessing that the compile
function gets tree shaken and nothing matches the node_modules/vue
test of the cache group anymore and no shared file is created.
Everything should be good now! The failing tests are unrelated (probably introduced by #865 since we use the |
@Lyrkan could you fix up the yarn.lock conflict one more time? Thanks! |
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.
I'm still reviewing, but it's looking really good so far!
configureOptimizeCssPlugin(optimizeCssPluginOptionsCallback = () => {}) { | ||
webpackConfig.configureOptimizeCssPlugin(optimizeCssPluginOptionsCallback); | ||
configureCssMinimizerPlugin(cssMinimizerPluginOptionsCallback = () => {}) { | ||
webpackConfig.configureCssMinimizerPlugin(cssMinimizerPluginOptionsCallback); |
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.
We'll need to summarize these changes in the CHANGELOG. If you could bootstrap that, it would be awesome - but I can also do it later.
} else { | ||
// see versioning.js: this gives us friendly module names, | ||
// which can be useful for debugging, especially with HMR | ||
optimization.namedModules = true; |
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 related to the versioning.js
above. Why is this not needed now? Is it the default? thx for the explanation :)
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.
optimization.namedModules
has been replaced by optimization.moduleIds: 'named'
: https://webpack.js.org/migrate/5/#update-outdated-options
This is the default value for development builds: https://webpack.js.org/configuration/mode/#mode-development
@@ -632,7 +542,7 @@ class ConfigGenerator { | |||
|
|||
buildWatchOptionsConfig() { | |||
const watchOptions = { | |||
ignored: /node_modules/ | |||
ignored: 'node_modules' |
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.
Can you explain this change?
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.
When I did that change using a RegExp was not allowed for that option anymore.
It seems that it was added back by webpack/webpack#11498.
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.
I see it! We should probably change this back then?
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.
FYI - this has been reverted
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.
Review complete! This is really nice, and it deletes more code than it adds - always wonderful :). There is one significant thing that I'm wondering about for Webpack 5 - Asset Modules - https://webpack.js.org/guides/asset-modules/
It seems like using file-loader, url-loader, etc for images, fonts, etc is now "non-standard", and instead we should use Asset Modules. If I'm correct, ideally we would refactor to use that immediately so that we can use the breaking changes in the release to remove file-loader, url-loader, etc. But, we could do this in a different PR.
A few other things I'm thinking about:
A) I'm wondering if we could start using webpack-manifest-plugin also to generate the entrypoints.json file. I have no tried it yet, but the idea would be to use the useEntryKeys
option. This would mean we would need to use the plugin 2 times... once for manifest.json and again for entrypoints.json... which I think is ok, but a bit non-standard. We might also (but not sure) need to remove or move the "sha-" checksums - I'm not sure if those could be done with webpack-manifest-plugin
B) A lot of defaults were changed on stats
. We should revisit if we can remove much/all of our "overriding" and allow encore to have a more standard output.
C) Finally, it might be nice to have a new method for the persistent caching - maybe .enableBuildCache()
, with some config to allow them to add more files / configure the cache. We could, by default, at least add webpack.config.js
to the cache... so it "should" work out of the box for most users.
Cheers!
Hm, I kept
Hm, I haven't really thought about that to be honest, in my opinion both plugins do different things.
👍
Things can easily go wrong with persistent caching... even only enabling it for the config file could lead to some issues if not done properly (for instance if the config use values coming from environment variables, which is far from an edge case imo: https://github.com/webpack/changelog-v5/blob/master/guides/persistent-caching.md#version). I also think that we should provide some way to enable/configure it in Encore but that should probably be treated separately after thinking for a while about how we really want to handle that. |
The migration page - https://webpack.js.org/migrate/5/ - suggests that we should migrate away to use asset modules:
So, I think we should make this change now (or at least try it). However, we can do it in another PR after we merge this. And we can keep using file-loader for
It's something we can investigate after we merge this PR. If we can make Encore smaller and more standard, cool :)
Concerns heard! Let's definitely consider it in another PR. It would be a nice "sweet new feature!" to have when we release the Webpack5-powered Encore ;). |
Nice!! :D |
… 13 support (weaverryan) This PR was squashed before being merged into the main branch. Discussion ---------- [Webpack5] Using old watchOptions regexp & removing Node 13 support 2 quick, minor follow-ups from #645. * Removed Node 13, as it is no longer supported * Changed the ignored back to a regular expression, which it was originally and once again a regexp works: #645 (comment) * Upgraded to Stimulus 2, which the stimulus bridge now depends on Commits ------- 816003e updating hashes for latest deps ad714b7 upgrading to stimulus 2 3a4005c upgrading deps 4e0aa70 fixing tests 3c43cea removing support for Node 13 343a4fe changing watch options ignored back to a regexp to match previous behavior
… (weaverryan) This PR was squashed before being merged into the main branch. Discussion ---------- Updating images and fonts to use Webpack 5 Asset modules This is one more big piece for proper Webpack 5 support - see #880 This also updates the CHANGELOG both for this PR and for the original in #645 And, this adds a new assertion method that helps check directory contents in functional.js in a way that is more resilient to hash changes. For the most part... this just worked! See the CHANGELOG. Because this will release on 1.0, I have decided to remove a few methods instead of deprecating them ). Cheers! Commits ------- 55495cf changing the default to asset/resource a663fbc moving require into loader function 8d3b401 fixing tests on the lowest deps due to version upgrades 3e9c787 Making tests more resilient to failure by fuzzy checking some hash filenames d86e115 upgrading Webpack min 64c6c2f updating CHANGELOG for original Webpack 5 changes in #645 16c0f12 Updating images and fonts to use Webpack 5 Asset modules
…pack 5 persistent caching (weaverryan) This PR was squashed before being merged into the main branch. Discussion ---------- [Webpack 5] Adding new enableBuildCache() method for Webpack 5 persistent caching Hi! This is a very simple feature, but it can also easily be improperly used. It is part of #880. See some comments about it from @Lyrkan from #645: > Things can easily go wrong with persistent caching... even only enabling it for the config file could lead to some issues if not done properly (for instance if the config use values coming from environment variables, which is far from an edge case imo: https://github.com/webpack/changelog-v5/blob/master/guides/persistent-caching.md#version). The trick, for us, is to: A) Make sure we are doing everything as correctly as we can. For example, I assume that WE don't need to include `.babelrc` or `postcss.config.js`, but I actually don't know that for sure. Should we also potentially be adding Encore itself to the build dependencies? B) Good communication in the documentation above the method and in the (eventual) recipe where we include this in the user's webpack.config.js file ## TODO * [ ] 1) Test in a real project to see if we're missing anything (also play with what the valid keys are under `buildDependencies` - other than `config`, this is not documented anywhere). * [x] ~~2) Check into Stimulus: I'm curious if `controllers.json` will need to be added to the build dependencies. I'm also curious if the UX vendor libraries will need to be in the build dependencies, as these do not follow the normal versioning (i.e. their directories in `node_modules/` can change without a change to `yarn.lock` or `package.json`)~~ should be solved by linked changes in #888 Cheers! Commits ------- a592c82 lint 9f117a4 fixing test 7a39654 Adding new enableBuildCache() method for Webpack 5 persistent caching
This PR was merged into the main branch. Discussion ---------- Allow node 10.19 This was bumped in #645 Ubuntu LTS [ships node 10.19](https://packages.ubuntu.com/focal/nodejs) by default. If we can, let's save ppl (aka me) from installing yet another ppa. Commits ------- 7721aa2 Allow node 10.19
Last week Webpack added a compat layer to its v5 alpha that allows the
mini-css-extract-plugin
to run on it (with deprecated messages). Since we include that plugin by default (and a lot of our tests relie on it) it was the main thing blocking us from preparing the migration.I basically started from #564 which was updating some dependencies, enabling CSS HMR when needed and adding a
configureMiniCssExtractPlugin(...)
method, but with a few changes:enableVersioning applies to js, css & manifest
test: it seems to be working fine by defaultwebpack-manifest-plugin
into our code: Incompatibility with the latest version of mini-css-extract-plugin (0.4.3) shellscape/webpack-manifest-plugin#167 is still an issue but @mastilver has been working on the project lately (which is why the plugin works with Webpack 5) and a fix has already been merged on thenext
branch, so it's probably only a matter of time now :). Edit: Fixed inwebpack-manifest-plugin@^3.0.0-rc
So now, about the state of that PR:
0 failing test left:
6 tests that will probably be fixed by Fix InnerGraph webpack/webpack#10661 in[email protected]
:Uncaught Error: Error when running the browser: Error: Error when running the browser: ReferenceError: mod is not defined
All the 7 tests related to thevue-loader
are failing with aCannot find module 'webpack/lib/RuleSet
error message (see: Webpack 5 compatibility: Missing module RuleSet vuejs/vue-loader#1599).1 test related to thewebpack-manifest-plugin
issue previously mentioned1 test related tocreateSharedEntry()
which doesn't seem to work properly1 test related to Babel that doesn't transform an arrow function as expectedA lot of deprecation notices (but most, if not all, of them are triggered by vendors), for instance:
Compilation.hooks.normalModuleLoader was moved to NormalModule.getCompilationHooks(compilation).loader
Module.id: Use new ChunkGraph API
Module.updateHash: Use new ChunkGraph API
Chunk.modulesIterable: Use new ChunkGraph API
ChunkGroup.getModuleIndex2 was renamed to getModulePostOrderIndex
Compilation.chunks was changed from Array to Set (using Array method 'reduce' is deprecated)
chunk.files was changed from Array to Set (using Array method 'reduce' is deprecated)
Some modules do not declare they are compatible with Webpack 5 yet (warning messages during
yarn install
): this shouldn't be an issue unless those modules require a major version upgrade to be officialy compatible (in which case breaking changes could impact us).We're still using
webpack-cli@3
which may not support Webpack 5. It currently seems to be OK but we should probably upgrade towebpack-cli@4
(currently in beta). I took a quick glance at it and it probably won't be an easy thing to do, mainly because of how our "runtime context" works and how the new version of the CLI calls Webpack (through another process).