fix(Server): disable page reload when hot is not specified (options.hot)
#1276
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1276 +/- ##
==========================================
+ Coverage 75.72% 75.77% +0.05%
==========================================
Files 5 5
Lines 482 483 +1
Branches 156 155 -1
==========================================
+ Hits 365 366 +1
Misses 117 117
Continue to review full report at Codecov.
|
|
@byzyk I have checked out your fork by defining the following in my package.json: And I am receiving the following error when running webpack-dev-server: Versions (working combinations): Which webpack-dev-server version did you base your fix from? And which version of webpack do you have? Update: I have done a clean install of my npm packages, by clearing the cache and deleting node_modules, and the same error appears. |
|
@benstaker I tested it with This might be related to code builds. As per contributing guide, try this step before testing:
Let me know if that helped |
|
@benstaker I had the same issue at first. I then did: And I was able to test the fix. |
|
@byzyk @happypoulp thank you both. I have tried running Side note: The same error is outputted when running It seems that I am having a different result to you both, may be due to the way I have defined and installed byzyk's forked copy (see my previous comment re. package.json). It's missing the |
|
@benstaker did you run I used the URL you provided in my package.json to test this PR on my side, so this part should be fine. |
|
@happypoulp I have tried running I have now downloaded the zip of branch I have tested running |
|
@benstaker I did not had those error in my JS console. Maybe the webpack config we use to test this PR differs somehow... const path = require("path")
const webpack = require("webpack")
const HtmlWebpackPlugin = require("html-webpack-plugin")
module.exports = (settings) => {
return {
entry: {
app: [
path.join(process.cwd(), "src/index.js"),
],
},
output: {
filename: "[name]-[chunkhash].js",
path: path.join(process.cwd(), "build"),
publicPath: "/",
},
devServer: {
contentBase: "./build",
disableHostCheck: true,
historyApiFallback: true,
host: "127.0.0.1",
port: 8181,
},
resolve: {
symlinks: false,
modules: [path.join(process.cwd(), "src"), "node_modules"],
},
module: {},
plugins: [
new HtmlWebpackPlugin({
filename: "index.html",
template: "index.html",
inject: true,
minify: false,
}),
],
}
} |
|
@happypoulp quite possibly, thank you for checking! This may be due to the v2 webpack and latest webpack-dev-server I am using. We're using this combination because v3 webpack is currently too slow for incremental builds, so have reverted to webpack v2. It seems that this issue is now solved, so please continue 🥇 |
|
@shellscape seems like this PR resolved the issue. Would you be able to review and give your feedback? I see that some tests are failing which is weird since overall coverage increased. |
|
Note that full page reload ("live reload") and HMR are two distinct features, see here: #1251 (comment). They are used in different scenarios and one can not completely replace another. I.e. you can update JS/CSS/images with HMR, but if you developing static HTML page, you need to update its HTML with a full page reload ("live reload") because this is the only way to do it and HMR will not work in that case. Moreover, currently due to a bug, full page reload on HTML change is actually only works with HMR turned off, see #1271 . So your proposed solution will completely disable HTML refreshing in the described scenario, with HMR turned on or off. The correct way to proceed on this is to implement a new option for "live reload", like you mentioned. The source files changes need to be delivered to the browser somehow. The simplest way to do it is a "live reload". On top of it you can use HMR, then JS/CSS/images will use it, but HTML changes still will need to be refreshed with "live reload". And if you don't want any of this you just turn off HMR and "live reload", but not turn off "live reload" by turning off HMR which is what you proposing |
|
@andreyvolokitin I agree, I also feel like However, the proposed solution won't disable |
|
Live reload for changes in HTML is currently not working with |
|
@TheLarkInn would you be able to review and give a feedback whether this PR is ok to merge? |
For additional info, see: https://github.com/webpack/docs/wiki/webpack-dev-server#automatic-refresh
|
|
@andreyvolokitin please refer to PR description. Suggested changes don't remove so-called As per #1251 , the issue was that page kept reloading even when As we already discussed, currently there is no option to explicitly enable Does that address your concerns or did I miss something? |
|
Yes, you keep missing the point of live-reload. Live-reload is not an addition to HMR. It is the other way around: HMR is an addition to live-reload. And all in all, they are two separate features. Live-reload is always enabled and can not be disabled currently (again,
Your changes remove live-reload as a basic page refreshing mechanism, which is currently always enabled and is also active with
This is intended behavior, because by design
I think it isn't.
Actually, there is: live-reload is always enabled, and if HMR is disabled with
As stated in the docs, live-reload is not a fallback for HMR, but HMR is an additional feature on top of live-reload. Live-reload is called "automatic refresh" there and has two modes: "inline" and "iframe". |
|
@andreyvolokitin thanks for your comment. Let me clarify a bit. I can't remember that I've ever said Your concern is that this PR is aimed to remove While I appreciate your input, I don't get it why are you so worried about Just as a side note, you're referencing docs (which might not be that precise and could be outdated as well), while I'm trying to back up my statements from what I've seen in the source code. Thus, I don't agree with you on Anyway, let's wait for the webpack maintainers (@sokra @TheLarkInn @shellscape) to jump in and will see how we should proceed with this one. Otherwise, we can keep this argument going on forever while basically saying almost the same stuff just in different words. Peace ✌🏽 |
Should have major label (and will be in next branch, not in master). |
Because of this: #1276 (comment)
I also have seen the code. From this code perspective, it seems to me that in case of |
|
@evilebottnawi thanks, will make those amends when I get a chance. What is your view on the discussion above? Does it make sense for page reload to be a distinct feature with its own option like |
|
@andreyvolokitin I don't see how #1271 is relevant to this. Originally this PR was meant to address #1251 |
|
@byzyk for me yes, i have some project on |
|
@byzyk PR is disabling live-reload if |
|
@andreyvolokitin actually in this PR page refresh should work with |
|
@byzyk yep, sorry, it is working, my bad. But now there is no HMR for css and js with |
|
I saw the code and what you changed from the beginning. I still think that you misinterpreted the meaning and intention of "hot" and "hotOnly" options. In your PR the code is doing 1) HMR with |
|
@evilebottnawi do you still think the approach I took here is the right one to move forward with? Based on the discussions above I feel like we should distinguish between |
michael-ciniawsky
changed the title
hot is not specified (options.hot)
Codecov Report
@@ Coverage Diff @@
## master #1276 +/- ##
==========================================
+ Coverage 75.72% 75.77% +0.05%
==========================================
Files 5 5
Lines 482 483 +1
Branches 156 155 -1
==========================================
+ Hits 365 366 +1
Misses 117 117
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1276 +/- ##
==========================================
+ Coverage 75.72% 75.77% +0.05%
==========================================
Files 5 5
Lines 482 483 +1
Branches 156 155 -1
==========================================
+ Hits 365 366 +1
Misses 117 117
Continue to review full report at Codecov.
|
|
/cc @byzyk sorry for big delay, i think you are right, we need:
Summary:
Also we deprecated What do you think? |
|
+1 for an option to disable the full page refresh. There's a bit of discussion here too: #1744 tl;dr: Our codebase doesn't work with HMR, so I'd like to disable it, but that enables the full page refresh mechanism. Our UI is expensive to load, so reloading the page on every file change is extremely disruptive. It would be incredibly useful to have webpack build on every change, but I control when the page reloads. |
|
Hi @evilebottnawi! Sorry for a delay, yep that's something I had in mind originally and I guess that's exactly what other folks are asking as well. |
There was a problem hiding this comment.
IMO there is already a huge delay. @evilebottnawi it still appears to be relevant. I have many a times realised that there must be an option to turn page reload off when not using hot.
|
Here answer #1276 (comment), this PR is abandoned so feel free to send a new PR |
|
Hi everyone! Sorry guys but unfortunately I won't be able to finalize this one because of the current workload. I'll try to revisit it in a month or so. In the meanwhile feel free to pick up where I left or simply submit new PR since codebase has evolved a lot. |
|
Implemented you can disable hot using |
|
One or both of [At this point I change and resave a source file...] Then the browser reloads, losing all state. Is that as expected? |
For Bugs and Features; did you add new tests?
No
Motivation / Use-Case
Aimed to fix #1251. As per the issue, the page keeps reloading even if
hotoption is not specified orfalse. This PR contains fix which disables page reload whenhot: falseor not specified orhotOnly: true. To enable page refresh along with HMR one has to explicitly sethot: true.Breaking Changes
No
Additional Info
I think we can consider adding one more option, something like
livereloadfor use-cases when people don't wanna use HMR but instead prefer to go with a regular page refresh. I didn't do it as I feel it's more like a new feature while I tried to keep this PR as simple as possible.