Overlay blocks hot-reload refreshing page
Closed this issue · 8 comments
For some reason the presents of the overlay entry results in hot-reload not refreshing the page when it needs to:
This is what a normal hot-reload looks like in the console:
But with the overlay:
I suspect that the hot-reload system works by having a global try/catch that the overlay bundle isn't included in, so it doesn't think the hot-reload errored out like normal.
It seems that a work around for this problem is to import the overlay into your app (similar to babel-polyfill):
if(process.env.NODE_ENV === 'development') {
require('webpack-serve-overlay');
}
I'll update the README.md to mention this, as for now it means the overlay will function as before without issue.
When I get a chance I'll look into why this is happening, and how to solve it so users can switch back to including the overlay as an entry in their webpack config.
I think the importing solution above is actually the better way of using the overlay, as it sidesteps a lot of the problems that entry (& automatic entry insertion) brings.
I'm keeping this issue open for now, as I'm still interested in figuring out why it breaks the hot-reloading, and what the proper steps to make it play nice, just for any future development in this area.
@G-Rath I think it would be good to move the conditional NODE_ENV check into the library itself so that the developer doesn't have to think about it, similar to how react-hot-loader works.
With that built-in, I believe that the entry config would work (that's how I'm using it at least, I have the 3 lines as a separate file loaded into the entry), or at the very least that would just be a one-line import with no extra code.
This would also make it easier to show any errors that may pop up before the require due to ESM hoisting of import statements.
@agilgur5 I do like the entry config idea, however I'm torn on moving the NODE_ENV check.
The whole idea with the check is that it's not actually part of the library itself: How you use the overlay is up to you. If you want to include the library in production, so be it.
The example with the NODE_ENV check is a method of including the library in a manner that lets Webpack completely axe it from the bundle, which I don't think it'll be able to do if the check is in the file (as I suspect it won't try and analyse the methods to see if they're used, and instead just opt to leave them in - I could be wrong on this however. I also know that the bundle has an insanely small footprint in the schema of things - I raise this point first mainly to get it out of the way).
I've already seen one library that includes the overlay without any conditional checks, along with another that does their conditional checks seperately.
I love the idea of only having to use a single entry line, and I had completely missed that import statements are hoisted, so it would be great to find a way to have it loaded first, as you say.
It could be a matter of adding another env variable as an override, something like SERVE_OVERLAY_CHECK_FOR_PRODUCTION, but I'm not wanting to bloat the library with a lot of variables if I can avoid it (at the current time - a lot of this depends on some of the possible upcoming changes, where the overlay might get a bit of a structural refactor).
I'll definitely keep this in the back of my mind for when me & @shellscape look to add new features, as that may lead to us splitting the package up into a couple of files, allowing you in turn to import different files depending on what you wanted (i.e the main file could be one with the production check, which literally just imports the websocket.js file - that way development libraries like the ones mentioned above could instead import the actual main file to not have the check).
@jxnblk & @pedronauck if you don't mind me roping you guys into this, I'd be interested in hearing your thoughts (if any), as while this change makes sense for the majority of app-focused projects, your projects seem to be more for assisting in developing apps themselves, and so represent the most common "edge" case use of the overlay that this change would have an impact on.
Thanks for the detailed response @G-Rath !
The whole idea with the check is that it's not actually part of the library itself: How you use the overlay is up to you. If you want to include the library in production, so be it.
I suppose that's reasonable, it just wouldn't be a best practice to use webpack-serve in production.
It's also worth noting that this library will error unless there is a hot client running, so if served in a bundle not served by webpack-serve it will throw a runtime error (e.g. using webpack -d --watch and loading the bundle with this library included gave me a runtime error, just doing webpack -p without a check should also error). This is perhaps worth a separate issue if deemed as unintended behavior.
The example with the NODE_ENV check is a method of including the library in a manner that lets Webpack completely axe it from the bundle, which I don't think it'll be able to do if the check is in the file
It will still be axed from the bundle if using DefinePlugin and a minifier (which are by default included in webpack's production mode, at least in 4.x). Many libraries, including React itself utilize this pattern already. This does of course depend on this library's build output to still include the check, i.e. don't use DefinePlugin + a minifier here.
I love the idea of only having to use a single entry line, and I had completely missed that import statements are hoisted, so it would be great to find a way to have it loaded first, as you say.
Not something I thought of either until webpack/webpack#2776 haha. Based on my usage, it seemed like this library will not overlay an error if it occurred on the very first build before the require statement, but if it was successfully imported once it will still show all subsequent errors (I assume it ends up still working because once it's in the browser once it doesn't need to be required again), so it can be hard to notice.
It could be a matter of adding another env variable as an override, something like SERVE_OVERLAY_CHECK_FOR_PRODUCTION, but I'm not wanting to bloat the library with a lot of variables if I can avoid it (at the current time - a lot of this depends on some of the possible upcoming changes, where the overlay might get a bit of a structural refactor).
I think having a separate entry file here that includes the check makes sense and is the simplest option that doesn't change other behavior (as opposed to within the file).
Another option that might make sense might be to load as a middleware and have a runtime config similar to webpack-serve-waitpage, but I'm not sure of the complexity of that compared to the other options discussed in this issue / other issues
I'm not totally sure if that separate file will work as a pure import without being in the entry array though, as it would still have to use require in a conditional (vs. if it were in the one file itself, there would be no conditional require statement, but I'm not sure my interpretation of load order is correct on this and can't find too much online). Allowing developers to use a pure import vs. as an entry file might entail different considerations, but it isn't necessary to have in any case if another option is given.
Thanks for the detailed response
Thanks for getting involved, and bring some interesting points to the table @agilgur5 !
it just wouldn't be a best practice to use webpack-serve in production
That depends on your definition of "production" - case & point is the two packages that I referenced: They have a legitimate use case for using the overlay in production, as their production is as a development app for other devs to work with.
this library will error unless there is a hot client running
Totally absolutely definitely make a new issue for this - please & thank you! I'll say that for now it's undecided behaviour, until I've had a chance to check it out myself, but ideally it shouldn't crash everything if a hot-client isn't running. This could just result in "don't use it without a hot-client", but still merits investigation so I at least know whats actually happening.
It will still be axed from the bundle if...
I know that's the theory, but I've read some interesting things of late (that I've not had a chance to properly investigate) about that while Webpack in theory should be able to chuck out a lot of code, it seems that in practice it doesn't always throw out what you think it would. It's by far not the biggest deal, but just something I want to have a play around with myself to get some clarity on what sort of code Webpack can & can't deal with. I won't block any changes b/c of this point, and mention it just to indicate what my (current) understanding is.
I think having a separate entry file here that includes the check makes sense
I agree - for the time being I think I'll make up just a simple 'onlyIfProduction.js' file that has the check, and release that. Then when time permits, look into the other suggestions we've talked about.
Another option that might make sense might be to load as a middleware
That's what I was thinking, but it'd be dependent on some of the new ideas & features shellscape has in mind, as like you said adding it now would just introduce unnecessary complexity for something that's of little gain.
Along with that, I did initially look into using a middleware, but don't think it'll actually work: webpack-serve-waitpage works by just serving an entire html page. For this library, the middleware would have to inject JS code when certain requests are made for specific files - while I've not had a lot of experience with middlewares + node servers (so this might actually be really easy..?), I figured it'd get really messy really fast, as well as very buggy and error prone.
I'm not totally sure if that separate file will work as a pure import without being in the entry array
I'm not entirely sure what you mean by this, but I think I know what you're getting at - I've not done anything with the entries field, so i definitely need to have a play around before diving in further.
However, at the very least, you should be able to do the check in the webpack.config itself, since it's JS and an array - just include the entry if you're not in production. Depending on what webpack does with undefined in the array, it could possible even be a function exported by the package: require('webpack-serve-overlay/returnEntryOnCondition')(NODE_ENV !== 'production') or similar (with NODE_ENV !== 'production' being the default value for the function).
I'm a little overworked right now, so forgive me if I've missed a point or anything, but I'm very excited - this is some great stuff 😃
That depends on your definition of "production" - case & point is the two packages...
Ah, I didn't look at those links besides the lines you referenced. The term "best practice" effectively means the "best option for the most often used scenario", but maybe we have different interpretations of that term. Indeed, those two packages aren't the "wrong" way of doing it, they just serve different means / purposes and therefore deviate from the common usage. 👍
Totally absolutely definitely make a new issue for this...
Done, see #8 . Not a big error as it's just because __hotClientOptions__ is undefined if there is no hot client running, so no need to investigate :) Not really sure how you'd want to address that (early return if no __hotClientOptions__? set a default __hotClientOptions__?) and haven't dived into the code just yet myself.
I know that's the theory...
Can't really tell what you're talking about without a link, but yea tree shaking (usually part of the bundler, i.e. Webpack, Rollup, etc) and dead code analysis (usually part of the minifier, i.e. Uglify, etc don't work in all cases as they might not know enough to guarantee the code is unused. Tree shaking specifically relies on ESM imports and asks for you to label code as with or without side-effects. It's probably best to trial-and-error with some minification options turned off (i.e. no mangling and such so it's still readable). Could potentially add a diff test if that were important enough though I've never had to deal with the topic to such an extent to really give good advice on it.
I agree - for the time being I think I'll make up just a simple 'onlyIfProduction.js'
Done, see #9 .
Then when time permits, look into the other suggestions we've talked about.
Makes sense to me 👍 . Those are more complicated and all of the above are just to enhance usability, so probably an nbd for most people
...For this library, the middleware would have to inject JS code when certain requests are made for specific files...
Yea I was thinking a middleware could just inject a script tag if any HTML is being returned in the most naive case, which should simplify things quite a bit. idk what types of edge cases one might want to handle in more complex cases, but that's what issues and iteration are for.
I'm not entirely sure what you mean by this...
I meant if someone were still going to import this into their code, as opposed to adding it as an entry. I think if one added import 'webpack-serve-overlay/onlyInProduction' to the top as their first line of code it would load first, but not sure. My testing was inconclusive since a failure to compile on the first compilation doesn't show an overlay for me regardless of set-up. Potentially if I had a separate entrypoint and separate script tag instead of one entrypoint with an array and one script tag it would show up. My entry config looks similar to main: ['webpack-serve-overlay/onlyInProduction', './main.js'] so the overlay code runs first, but is in the same bundle as my other code. Separate entry is probably a better option
EDIT: I definitely think having it as a separate entry is the ideal usage as separate entrypoints / script tags ensures the overlay always shows up as its separate, isolated bundle will never fail to compile. In that case, excluding the overlay script tag from production HTML would also be ideal to have as little production impact as possible. Even with onlyIfProduction, webpack's bundling code will still create a tiny bundle of JS that does nothing. That's not optimal, but not particularly harmful either. A middleware would remove the need to have a conditional script tag for optimal usage and the need for a separate entry file for normal usage.
However, at the very least, you should be able to do the check in the
webpack.configitself
Yea of course that's another option available. require('webpack-serve-overlay/returnEntryOnCondition')(NODE_ENV !== 'production') is probably more effort than worthwhile since it still requires JS and is therefore not much different from just including the if statement (or ternary) yourself. Honestly probably more confusing than just having an if statement. Might actually be more characters than the if statement too haha 🤔
Just an update here, similar to my edit above, that the ideal usage in my opinion is to use with html-webpack-plugin and use:
entry: {
// don't include overlay in production
...(isProd ? {} : {overlay: 'webpack-serve-overlay'})
}
which would obviate the need for onlyIfDevelopment; but not everyone can be expected to use html-webpack-plugin so I do think the option has its uses