-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
[WIP] Use mini-css-extract-plugin@^0.7.0 and embed webpack-manifest-plugin into Encore #564
[WIP] Use mini-css-extract-plugin@^0.7.0 and embed webpack-manifest-plugin into Encore #564
Conversation
Looks like there is an issue with the hashes in filenames, it doesn't use the same ones for my env (on which tests are all greens), Travis and AppVeyor. I'll try to see if I can find out why... |
I think that's related to webpack-contrib/mini-css-extract-plugin#308 The issue was introduced by Let's put this PR on hold for now :) |
92017f7
to
9fe1632
Compare
In the meantime I enabled HMR on the Encore.configureMiniCssExtractPlugin(
loaderOptions => {
// change loader options here
},
pluginOptions => {
// change plugin options here (optional)
}
); |
@tianyong90 You should actually already be able to use HMR with CSS by disabling the part that extracts styles to files, for instance: if (Encore.isDevServer()) {
Encore.disableCssExtraction();
} |
@Lyrkan Thank you! I just start using encore yesterday (in fact I have stared it for nearly a year), I use it in my Laravel project and I think it much better than laravel-mix which I used for years. 😄 I've tried just now, but it did not work yet. I modified my css and the hot-update.js file loaded successfully, however the page style did not change. |
This need to be documented IMO |
I'll have to take a look at it but last time I tried doing that it was working fine.
I was planning to add a boolean parameter to Encore.disableCssExtraction(Encore.isDevServer()); Merging this PR would probably a best solution for HMR but I think the inconsistent hashes issue hasn't been fixed yet on |
I'm not really a big fan. I can understand this parameter is here to not breaking methods chaining, but I really think adding a enable/disable parameter to a method that disable something is wrong. Instead, maybe we can do something like |
I read it more like:
Kind of the same way we already have |
835b2ed
to
2e1122e
Compare
Rebased and changed the
|
2e1122e
to
d3adb3a
Compare
…trict regarding hashes
099046e
to
0ae3eff
Compare
Still an issue... so I added a commit that makes the failing test less strict regarding hashes: it will still check that the expected files are generated but all hashes will be accepted. I'm not sure that's the best thing to do since having inconsistent filenames between environments can be really annoying... but that's an option if we really want to update the |
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.
Sorry for letting this sit. I just addressed the hash test problem.
In general, does this have anything blocking it? What's the status of the HMR? About the disableCssExtraction(true)
thing (because it would indeed be confusing), one option is to use enableCssExtraction()
or enableCssExtraction(false)
. If we did this, we should probably deprecate the automatic extraction of CSS files. What I mean is, if you don't explicitly call enableCssExtraction()
, we would trigger a deprecation warning. The idea is that we would make this part of THEIR config so that it's more explicit.
But... I only want to do this if there is a good reason to disable extraction (like, HMR basically does NOT work without it). And, I'd probably rename it to extractCssToExternalFiles()
. But mostly... I would just like HMR to work without the user needing to worry about this ;).
} | ||
|
||
return 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.
So the unstable hashes only affect this test? Is it the only one where we have versioning enabled and are checking for CSS paths? If so, I'm ok cool with this... it's not ideal... because we'd like to know if hashes are changing unnecessarily... but we need to be pragmatic and nobody can think of a better solution :).
Could we refactor this into a reusable function... more for clarity than reusability - something on webpackAssert
?
Hey @weaverryan,
Depending on who you ask the hashes could be considered a blocking thing since it prevents having reproducible builds under certains conditions. But that's the only negative thing I see right now.
Last time I checked it was working out of the box when using
I'm not sure adding mandatory methods is a good idea... especially since you could be using Encore to only handle JS files, in which case it wouldn't make sense to be forced to setup a CSS-related thing.
That shouldn't be needed for HMR if this PR is merged, and if someone really has to do it for another reason that's still doable using |
Closing this PR in favor of #645 since it doesn't seem to have the annoying inconsistent hashes issue. We could always reopen it if the other one doesn't get into a mergeable state fast enough. |
This PR was squashed before being merged into the main branch. Discussion ---------- Update Webpack to v5 (+ other dependencies) 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: * no more inconsistent hashes checks for the `enableVersioning applies to js, css & manifest` test: it seems to be working fine by default * no more embedding the `webpack-manifest-plugin` into our code: 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 the `next` branch, so it's probably only a matter of time now :). **Edit: Fixed in `webpack-manifest-plugin@^3.0.0-rc`** * removal of Node 8 compatibility So now, about the state of that PR: **0 failing test left**: * ~6 tests that will probably be fixed by webpack/webpack#10661 in `webpack@5.0.0-beta.15`: `Uncaught Error: Error when running the browser: Error: Error when running the browser: ReferenceError: mod is not defined`~ * ~All the 7 tests related to the `vue-loader` are failing with a `Cannot find module 'webpack/lib/RuleSet` error message (see: vuejs/vue-loader#1599 * ~1 test related to the `webpack-manifest-plugin` issue previously mentioned~ * ~1 test related to `createSharedEntry()` which doesn't seem to work properly~ * ~1 test related to Babel that doesn't transform an arrow function as expected~ **A 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 to `webpack-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). Commits ------- 57f64fa Remove unusued files/constants related to versioning/shared entry 8d7843a Add removed Babel test back 216a5ca Update css-loader and style-loader ddcd6d8 Merge branch 'main' into webpack5 444c37f Merge branch 'main' into webpack5 b292e76 Fix linting issues d576b21 Remove deprecation caused by the DeletedUnusuedEntriesJSPlugin b0f7190 Update some hashes in functional tests 4f6171b Update WebpackManifestPlugin to v3.0.0 and vue-loader to v16.1.0 9af90ee Remove wrong comment 50cea18 Bump min. Typescript version to 3.6.3 1d22520 Don't set hmr option for mini-css-extract-plugin (deprecated since 1.0.0) 564b147 Update Vue.js dependencies 8da531e Replace optimize-css-assets-webpack-plugin by css-minimizer-webpack-plugin f3843ca Update Webpack to 5.0.0
A while ago we were forced to pin
mini-css-extract-plugin
to<0.4.2
because>=0.4.3
is not compatible with the current version ofwebpack-manifest-plugin
(see shellscape/webpack-manifest-plugin#167).Since the
webpack-manifest-plugin
hasn't been updated for quite a long time I suggest that we go back to what we did previously: embed it directly into Encore and apply the patch from shellscape/webpack-manifest-plugin#176.At the same time the PR will:
mini-css-extract-plugin
to^0.6.0
^0.7.0
--hot
)configureMiniCssExtractPlugin(...)
method that allows to configure both the loader and the pluginIn terms of BC, it could have some impacts since the plugin won't be an instance of
webpack-manifest-plugin
anymore. Themini-css-extract-plugin
part however doesn't seem to introduce any BC break (only bug fixes and new features).Closes #415