Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat: support hot module replacement (HMR) #457

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions hotModuleReplacement.js
@@ -0,0 +1,19 @@
module.exports = function(compilationHash, publicPath, outputFilename) {
if (document) {
var origin = document.location.protocol + '//' + document.location.hostname + (document.location.port ? ':' + document.location.port: '');
var styleSheets = document.getElementsByTagName('link');
for (var i = 0; i < styleSheets.length; i++) {
if (styleSheets[i].href) {
var hrefUrl = styleSheets[i].href.split('?');
var href = hrefUrl[0];
var hash = hrefUrl[1];
if (hash !== compilationHash && href === origin + publicPath + outputFilename) {
var url = href + '?' + compilationHash;
styleSheets[i].href = url;
console.log('[HMR]', 'Reload css: ', url);
break;
}
}
}
}
}
18 changes: 16 additions & 2 deletions index.js
Expand Up @@ -22,6 +22,13 @@ function ExtractTextPluginCompilation() {
}

ExtractTextPlugin.prototype.mergeNonInitialChunks = function(chunk, intoChunk, checkedChunks) {
if (chunk.chunks) {
// Fix error when hot module replacement used with CommonsChunkPlugin
chunk.chunks = chunk.chunks.filter(function(c) {
return typeof c !== 'undefined';
})
}

if(!intoChunk) {
checkedChunks = [];
chunk.chunks.forEach(function(c) {
Expand Down Expand Up @@ -313,7 +320,8 @@ ExtractTextPlugin.prototype.apply = function(compiler) {
callback();
}.bind(this));
}.bind(this));
compilation.plugin("additional-assets", function(callback) {
compilation.plugin("before-chunk-assets", function() {
Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

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)

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..

Copy link
Author

@cletusw cletusw Apr 28, 2017

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!

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..

Copy link

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.

Copy link
Contributor

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.

// This appears to be the latest hook where the %%extracted-file%% and hash replacements work on initial load. Any later and the contents of modules appears to be sealed and changes don't have any effect until the next hot update.
extractedChunks.forEach(function(extractedChunk) {
if(extractedChunk.modules.length) {
extractedChunk.modules.sort(function(a, b) {
Expand All @@ -336,9 +344,15 @@ ExtractTextPlugin.prototype.apply = function(compiler) {

compilation.assets[file] = source;
chunk.files.push(file);

// Hot module replacement
extractedChunk.modules.forEach(function(module){
var originalModule = module.getOriginalModule();
originalModule._source._value = originalModule._source._value.replace('%%extracted-file%%', file);
originalModule._source._value = originalModule._source._value.replace('%%extracted-hash%%', compilation.hash);
});
}
}, this);
callback();
}.bind(this));
}.bind(this));
};
18 changes: 15 additions & 3 deletions loader.js
Expand Up @@ -10,11 +10,13 @@ var LibraryTemplatePlugin = require("webpack/lib/LibraryTemplatePlugin");
var SingleEntryPlugin = require("webpack/lib/SingleEntryPlugin");
var LimitChunkCountPlugin = require("webpack/lib/optimize/LimitChunkCountPlugin");

var extractTextPluginHmrRuntime = require.resolve("./hotModuleReplacement.js");
var NS = fs.realpathSync(__dirname);

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}`;
};

module.exports.pitch = function(request) {
Expand Down Expand Up @@ -120,8 +122,18 @@ module.exports.pitch = function(request) {
});
});
this[NS](text, query);
if(text.locals && typeof resultSource !== "undefined") {
resultSource += "\nmodule.exports = " + JSON.stringify(text.locals) + ";";
if(typeof resultSource !== "undefined") {
if (text.locals) {
resultSource += "\nmodule.exports = " + JSON.stringify(text.locals) + ";";
}
// module.hot.data is undefined on initial load, and an object in hot updates
resultSource += `
if (module.hot) {
module.hot.accept();
if (module.hot.data) {
require('${extractTextPluginHmrRuntime}')('%%extracted-hash%%','${publicPath}','%%extracted-file%%');
}
}`;
}
} catch(e) {
return callback(e);
Expand Down