-
Notifications
You must be signed in to change notification settings - Fork 512
feat: support hot module replacement (HMR) #457
Conversation
@@ -285,7 +292,7 @@ ExtractTextPlugin.prototype.apply = function(compiler) { | |||
callback(); | |||
}.bind(this)); | |||
}.bind(this)); | |||
compilation.plugin("additional-assets", function(callback) { | |||
compilation.plugin("before-chunk-assets", function() { |
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.
Without moving to an earlier hook, the %%extracted-file%%
, etc. replacements wouldn't take the first time around.
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.
Please add a comment and ideally a test to cover that.
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.
Comment added.
How would I do the test? It looks like there aren't any existing tests that check the contents of the JS file from which we're extracting. I could probably add the entire built file to the expected
directory, but then it would break every time webpack updates the prologue code.
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.
Maybe the question is what's worth testing. It could be possible there is no good way just yet.
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 change is the source of problem I mentioned in #457 (comment)
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.
@cletusw also, because of changing additional-assets
to before-chunk-assests
webpack does not produce source maps anymore :(
I am not sure how to fix this..
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.
@mieszko4 Yeah, I don't know enough about webpack or this code to know why one hook works and not the other, I just tried it on a whim and it fixed HMR. Since the maintainers are not planning on merging this PR I'm not inclined to work on it any further. Feel free to make a PR to my fork if you can come up with a good solution and I'd be happy to look at it!
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.
@cletusw I would help but unfortunately I do not know enough about webpack either..
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.
@cletusw by the way I was able to get "additional-assets"
to work so one hook could be used: https://github.com/faceyspacey/extract-css-chunks-webpack-plugin/blob/master/index.js#L331
To begin with I did it exactly as you did with both "before-chunk-assets"
callback and "additional-assets"
. In my case it was worse because I ended up duplicating a lot of code between the 2 callbacks.
Eventually however I went back and give it a try with the original callback and it worked. To be honest I'm not even sure what was different. My guess is it had to do with this line:
https://github.com/faceyspacey/extract-css-chunks-webpack-plugin/blob/master/index.js#L377
where I essentially overrode the _cachedSource
source, thereby hopping over any limitations in what you can do in each callback. I think likely once the source is cached it's sealed to future callbacks, and what I did to stripe out css injection (so that one of my 2 types of javascript chunks doesnt waste bytes having css in it) overrode that.
Basically what I did is likely a hack and not the way the designers of webpack would do this, which is why it's imperative they chime, examine the state of affairs, and give direction before merging things back into ETWP and risking incompatibility and other errors to its large userbase. And to be honest, I also think the plugin should be re-written from the ground-up. No offense to anyone, but it's impossible to understand what this code is doing without hours and hours of research (and I did just that; days and days of playing with it; I'm ashamed to say 2 weeks, 4 hours a day). The code is procedural with huge methods. No pure functions--and by that I mean these functions are sharing state/variables all over the place. It needs to be broken into many smaller well-named pure functions so anyone has any hope of understanding it. I also don't understand why no line breaks are used--there are no logical groupings of related code. To be sure, half the problem is that it was my first time delving into the in-memory structure representing your bundle, but no punches were pulled to make it readable to anyone without in-depth knowledge of Webpack. I'm not sure if the whole codebase is like this, but I'm seeing it's written in plain js with no transpiler. Not that transpiling is the save-all solution, but basically the code could stand to being upgraded to learning from idiomatic best practices over the last 2 years. I love webpack, but after looking at what was going on underneath the hood in a few of these plugins, I'm basically shocked. That it all works is a miracle. I think just what we're hearing in this issue shows how scared people are of dealing with its codebase. Like no other popular codebase in Reactlandia, everyone in the back of their head intuits that only @sokra 's knows how to deal with it. That's why this past year when other people stepped up it was so apparent that it was a big deal, i.e. something that needed to happen that was hard to happen. I assume they still have growing pains for days. A huge codebase was built by one person in an antiquated procedural style that does everything including the ceiling fan. There's no other way to put it. Hats off to @sokra for gluing this all together and making it work. But Webpack 3.0 is gonna need a major refactoring if it doesn't want to get beaten out by a team that takes all its lessons and does it more extensibly before it's 4.0. That all said, I haven't examined 95% of the code. I'm mainly going off a few plugins. So if they are anomalies, great! I don't know what else to say. I only share this because I think it needs to be heard. Maybe it's said a bunch elsewhere--I dunno, I try to stay focused in application code as much as possible vs. building/bundling. Everybody knows bundling/building is not sexy and fun to do--thats why I have nothing but profound appreciation that @sokra ever built this. ...anyway this is just a plugin; forget saving the world; this particular plugin should be built from the ground-up. If it was, the challenge of getting our PRs merged wouldn't be the problem it is. At the very least the person that built it simply should write a bunch of comments in the code.
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.
@faceyspacey I hear you. I think even Tobias agrees that the current implementation of the plugin is a hack. Note that webpack-defaults gives a transpilation step and makes it possible to rewrite a lot of code in a newer style. There's still the problem of legacy as you definitely don't want to break anything but it's a step to the right direction at least.
@bebraw Here's my attempt at adding HMR. Can you review? Thanks. |
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
==========================================
- Coverage 90.38% 81.26% -9.12%
==========================================
Files 6 4 -2
Lines 364 347 -17
Branches 77 73 -4
==========================================
- Hits 329 282 -47
- Misses 35 65 +30
Continue to review full report at Codecov.
|
@msuperina @jantimon Do you want to give this a go and let me know what you think? HMR for extract-text-webpack-plugin is here. 👍 |
hotModuleReplacement.js
Outdated
var hrefUrl = styleSheets[i].href.split('?'); | ||
var href = hrefUrl[0]; | ||
var hash = hrefUrl[1]; | ||
if (hash !== compilationHash && href === document.location.origin + publicPath + outputFilename) { |
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 will definitely break with different-origin public paths, but then again hot reloading overall wouldn't work in that case either.
location.origin
is also not available in IE.
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.
Fixed the location.origin
problem.
loader.js
Outdated
module.exports = function(source) { | ||
if(this.cacheable) this.cacheable(); | ||
return source; | ||
// Even though this gets overwritten if extract+remove are true, without it, the runtime doesn't get added to the chunk | ||
return "if(module.hot){require('" + extractTextPluginHmrRuntime + "');}\n" + source; |
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 and the other string concatenations could be template strings.
Is there any objection to using modern syntax in files that haven't yet been "converted"? I don't really see a reason to wait until a conversion; it's not like all features have to be adopted at the same time and with 100% coverage.
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.
Agreed. A string template would be far neater. Node 4 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.
I was originally hoping to get this into 1.x (this PR is off of the webpack-1
branch), which I'm assuming has to support Node <4. If that's out of the question, I suppose I can run off of my fork until I eventually get onto webpack 2.
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.
Ah, good call... You should definitely base this from master
.
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.
Changed base to master
.
Changed to template strings in 1c9a2ff. Is the code style okay? I figured real newlines in the code were better than \n
, but if I make the indentation match the surrounding code it'll appear with all those extra tabs in the built file itself.
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.
Match the current style for now. We have a stronger solution (webpack-defaults) in the pipeline that makes it easier to handle.
Hello folks, Any plans for merging this? |
We probably want final decision from Tobias before moving forward with this. |
I had a quick discussion with Tobias. Main points:
Based on this the only way I can see this type of functionality going in is through a plugin of a plugin (html-webpack-plugin style). If it's possible to expose enough hooks, then you could implement extract-text-webpack-hmr-plugin or so without growing the core apart from the plugin interface. |
I wish I had time to look into it properly. In my opinion dev configuration
should be as close as possible to production configuration. But as I said I
can only blame myself for not bringing a proper solution.
…On Mon, Mar 27, 2017 at 10:42 AM, Juho Vepsäläinen ***@***.*** > wrote:
I had a quick discussion with Tobias. Main points:
- ETWP is slow so even with HMR, it's not a good solution for
development.
- Instead, we should recommend development specific setup (['style-loader',
'css-loader']) (faster) and document that.
Based on this the only way I can see this type of functionality going in
is through a plugin of a plugin (*html-webpack-plugin* style). If it's
possible to expose enough hooks, then you could implement
*extract-text-webpack-hmr-plugin* or so without growing the core apart
from the plugin interface.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#457 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARdIg_SkNQ0MLlC0xym5Nd1hJ5U_ot-gks5rp4RrgaJpZM4MYXeL>
.
|
I fail to see how this is an argument against adding the option for HMR. I'm totally fine if the public recommendation is to avoid it and use a separate dev setup. I'm even fine if you have to pass ETWP an |
For anyone else who wants this feature, feel free to use my fork ( |
@cletusw It's not my call to make. Tobias made it clear to me HMR does not belong here. |
@bebraw here's something that might change the equation: Achieving the holy grail of Sever Side Rendering and Split Css Chunks. React Loadable is on course to serve up just the chunks (js + css) your current request needs (I'm working on this feature). However, this can't be a production-only thing, where you only finally see that your server-side rendered code splitting is working during production. So you need to be able to serve up css chunks and you need HMR to continue to work. This is actually 2 issues, and I've discussed elsewhere how to get the ability to have more than one extracted text chunk. So if we can merge such a PR in, you need to be able to have access to those chunks AND HMR at the same time during development. So @cletusw already built support for this. What's stopping merging his PR as an option. Not having it as an option is a major mistake. Perhaps @sokra isn't privy to the latest developments in server-side rendered code splitting. It seems to me an important direction that React + Webpack must finally go. As is, you can split chunks, but without significant work to incorporate chunks synchronously, a second request to get that chunk is needed, which also means any HTML it generates is not embedded in the initial page, which is bad for SEO. Contrary to what we might hope, code splitting isn't a solved problem (not outside Next.js at least). There is NO general and popular solution to embed required chunks (both JS + CSS) in the initial request for synchronous use client-side so React checksums match. There in fact is a lot of issues surrounding making this all work. I'm working on precisely this for React Loadable and am almost done. I have a few roundabout ways to make this all work that would be more seamless if ETWP extracted to individual chunks and if it supported extraction plus simultaneous HMR via the style-loader. The future of apps is code splitting everywhere, but that can't happen without server-side rendering addressed or if it's only a testable reality in production. |
@cletusw I started using your fork and it seems to almost be working. On changes to CSS files, I'm seeing
And based on examination of the CSS HMR aspect of your code: it's now supposed to look for the previous css file to replace. But how are those stylesheets supposed to initially get embedded in the page?? Here's my usage of the plugin in my webpack config:
UPDATE: I set It would be nice to actually make this work with individual css files generated for each chunk, as that is another feature I'm working on getting into this package or a fork. Perhaps that's more natural with how HMR is supposed to work anyway. |
@cletusw also, the extract styles file is no longer registered in the stats, e.g: |
@faceyspacey You need to include the CSS files manually in whatever As for the stats, I might not be able to get around to that for a while, but feel free to PR my fork! |
@cletusw Thanks so much for the response. Including the files manually is basically what I resolved to. That actually happens to work out perfectly in my unique scenario since I changed extract-textwebpack-plugin to produce multiple css files for dynamic chunks. A for fixing my issues with HMR, I actually was able to make your implementation work eventually. I then tried to make it work with my setup to produce multiple css files. For some reason, I had the following issue: HMR appends I know I'm introducing another aspect (creating multiple css files for dynamic chunks), but I'd love if you could take a look at what I've done. I built this to work with React Loadable by the way so all the css and js of dynamic chunks can be sent from the server for synchronously requiring on the client in one render. I have a real nice setup going right now--it just would be ideal to get the code back into this package. I'll send you my fork in a little bit. Basically the goal would be to make what I built work with filenames just based on |
@cletusw here's my package that provides css files per chunk and working HMR: https://github.com/faceyspacey/extract-css-chunk If you have some time, take a look at it, and then maybe we can discuss a version of these changes that can be merged into this package. |
@faceyspacey That looks awesome! |
Just to chime in, likely the best way to proceed would be to figure out how to expose an API you can hook into. After that it would need ETWP+ to work. That feels like the best way based on Tobias' decision. |
@bebraw ok, that shouldn't be too hard. But first we have these todos:
faceyspacey/extract-css-chunk@0739c47#diff-168726dbe96b3ce427e7fedce31bb0bcR313 That basically avoids merging all extracted css into one css file. I'm not fully sure I captured all possible cases by making that sweeping change. |
Hey guys, sorry for jumping in on the conversation, but doest HMR works with this plugin and if so where could I go and find a working implementation I could use as a reference or what do I need to change to make it work. Thanks. |
@cletusw up above provides a fork to use his HMR implementation: It works. Give it a try and report back if there are any problems. ...FYI, I took his implementation and made HMR work as part of a new package extract-css-chunk (linked above), which creates individual css files for all your chunks (for use with code splitting). |
@cletusw ...I was thinking back to what you said regarding:
That didn't affect what I was doing since extract-css-chunk's specific goal is getting those individual CSS files into the page, however that is not in fact how I made the following edit to your code to make that work
So notice how
any ideas how to automatically add this to the webpack bootstrap chunk? But it works like a charm, and you don't have to worry about adding the css file (possibly with a dynamic hash, which you need to get from stats) to the page. Perhaps this is less relevant in how you initially envisioned it where you have just one |
@faceyspacey Ummm, why do you need Also, this thread is getting significantly off topic. We can have further conversation about my fork or yours in an issue in their respective repos. |
@bebraw Thanks for all your responses, but at this time I'm not interested in adding additional complexity (both code-wise and for the user) to this by having it be a separate plugin-to-a-plugin, especially when the lack of HMR is explicitly listed as a shortcoming of the existing implementation. From the README: I'll continue using my fork in the hope that this PR will someday be merged. In the meantime, anyone else is free to take this work and attempt to implement it as a separate plugin to this plugin. |
@cletusw good point. The goal has been to have both However, having both solves a key problem when it comes to multiple css files generated from dynamic chunks: the fact that you're only embedding the needed css chunks for the initial request, whereas if the client later loads different split chunks, webpack doesn't know how to load both the js and css chunk files. So to handle that what I've done is create 2 js chunks:
And then what I have is React Loadable embedding the js chunk without the css in the initial HTML returned from the server. And then as more chunks are asynchronously loaded, the chunks with the css (provided via This allows for the smallest chunk sizes embedded in the initial request, i.e. no duplicate css. Anyway, that's how I stumbled upon using |
@cletusw Using your fork with combination of https://github.com/halt-hammerzeit/universal-webpack/ for some reason generates an empty |
I will close this, please release this PR as a separate plugin, the sole purpose of ETWP is to extract the CSS chunks from the bundle and emit a separate CSS file. For HMR in development enviroments |
@michael-ciniawsky I could not set |
@mieszko4 Please open an issue in regard to what currently doesn't work, when using
|
UPDATE: Here's the latest from extract-css-chunks-webpack-plugin HMR + Multiple CSS Chunks To immediately see it in action, try out the boilerplate: https://github.com/faceyspacey/flush-chunks-boilerplate Also note, it's part of a larger initiative revolving around React Loadable and its companion package 💩 Webpack Flush Chunks: https://github.com/faceyspacey/webpack-flush-chunks I'm open to merging code back into ETWP, but given how complex this code is (and it is), it really needs leadership from the original maintainers. Kickstarting the plugin of a plugin thing is something I'm willing to help anyone else who gets serious about it and wants to lead it, but I likely won't end up spearheading it, as it doesn't make sense to work on a PR that might not get merged. I think given @cletusw 's efforts which were not accepted, some effort from core EWTP maintainers in this direction makes the most sense as the next step (given we both have done a lot of work and demonstrated the base capabilities). I'm ready and willing should that time come. For now the best thing likely is to start using the extract-css-chunks package and make sure it's solid. Only then after a bunch of people use it we can think about merging back into ETWP. That's probably most natural and logical anyway. |
Fixes #30
Based on #89, https://github.com/TrejGun/extract-text-webpack-plugin/