Skip to content
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

fix: fix live reload for child compiler scenario #3972

Closed

Conversation

wood1986
Copy link
Contributor

@wood1986 wood1986 commented Oct 23, 2021

fix #3971

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Currently, webpack-dev-server only cares stats.hash but this hash does not count child compilation when you run the childCompiler after parent's seal stage.

Motivation / Use-Case

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #3972 (bd23be5) into master (8f66c22) will decrease coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3972      +/-   ##
==========================================
- Coverage   92.58%   92.28%   -0.30%     
==========================================
  Files          14       14              
  Lines        1362     1413      +51     
  Branches      474      521      +47     
==========================================
+ Hits         1261     1304      +43     
- Misses         93      101       +8     
  Partials        8        8              
Impacted Files Coverage Δ
lib/Server.js 93.58% <100.00%> (-0.29%) ⬇️
client-src/utils/parseURL.js 84.21% <0.00%> (-4.03%) ⬇️
client-src/utils/reloadApp.js 78.57% <0.00%> (-2.92%) ⬇️
lib/servers/SockJSServer.js 97.22% <0.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f66c22...bd23be5. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is valid solution, if something changed in child compilations, hash of main compilation should be changed. Adding applyEntryOption in processAssets hooks is too late, can you show me use case for this?

@wood1986
Copy link
Contributor Author

wood1986 commented Oct 24, 2021

What if child compilation depends on the chunk hash? For example service worker, SSR html page generation and so on.

My use case is the child compilation will do something similar to DefinePlugin to replace the token with the chunk hash. The compilation is using a different entry and different target.

I remember it was working before webpack-dev-server major update.

@alexander-akait
Copy link
Member

What if child compilation depends on the chunk hash? For example service worker, SSR html page generation and so on.

You have this problem due invalid usage of hooks, because it is too late for this...

@alexander-akait
Copy link
Member

alexander-akait commented Oct 24, 2021

If we merge we will always got reloading due changes in mini-css-extract-plugin and other plugins with child compilations, check it

@wood1986
Copy link
Contributor Author

Thanks, Let me read it first

@wood1986
Copy link
Contributor Author

wood1986 commented Oct 24, 2021

Here are my use cases and requirements

  • The child compilation needs to read parent compilation's finalized hash value of each chunk
  • Any change to the child compilation needs an auto-reload.

mini-css-extract-plugin's runAsChild is happened inside loader. But I do not want to use a loader and do not want consumers configure two things, loader and plugin.

if you are just talking about reload, I know how to do it. Again my child compiler needs parent compilation hash value result. How can I get the chunk finalized hash value before the seal?

@wood1986
Copy link
Contributor Author

wood1986 commented Oct 25, 2021

I am not sure if you are suggesting me I should use updateAsset to modify the asset at processAssets stage. If this is the case, it will be a bit difficult to parse and modify the asset at this processAsset.

Currently, I have several javascript parser hooks at the childCompiler.hooks.thisCompilation stage.childCompiler is waiting for the parentCompiler to finalize the assets and then read the parent asset filename to replace child source which is similar to what DefinePlugin is doing.

mini-css-extract-plugin can run childCompiler before processAssets because it does not depend on parent compilation stats/summary. So the parent hash can include child hash when child file is changed.

I also take a look how html-webpack-plugin does. It generates the html at PROCESS_ASSETS_STAGE_OPTIMIZE_INLINE and this generation is a separated process. Its childCompiler can run before processAssets.

In my case, I cannot because of using javascript parser hook in the child compilation

@alexander-akait
Copy link
Member

Again, if something was changed hash the main compilation should be changed, it is not only about live reloading, it is about caches and other mechanisms inside webpack, will break not only live reload, hot is broken, cache can be broken, emitting can be broken, please provide code of the plugin and I will show how to fix it

@alexander-akait
Copy link
Member

alexander-akait commented Oct 25, 2021

mini-css-extract-plugin's runAsChild is happened inside loader. But I do not want to use a loader and do not want consumers configure two things, loader and plugin.

You can configure loader inside plugin (and pass options to loaders too), we do the same in many places

@wood1986
Copy link
Contributor Author

https://github.com/wood1986/demo. Here is my sample code.

  1. run yarn runt app:start
  2. touch index.node.js file
  3. you will not see a auto-reload

@alexander-akait
Copy link
Member

Why you not emit files using webpack API?

@alexander-akait
Copy link
Member

Firstly as you can see you use parser.state.current.buildInfo.cacheable = false and I think you understand that is hack (I already say about cache problems above and you just), you can use it using https://github.com/webpack/webpack/blob/main/lib/optimize/ConcatenatedModule.js#L774, just add all files to your deps and you can cache.

@alexander-akait
Copy link
Member

alexander-akait commented Oct 25, 2021

Secondary, try parser.state.current.buildMeta = { hash: Math.random() }; note - it is pseudo code, ideally just calculate hash of content of your files and set this to prevent different hashes between builds.

There are no problems with dev server, webpack just can't understand your changes

@alexander-akait
Copy link
Member

alexander-akait commented Oct 25, 2021

Oh, I was wrong about parser.state.current.buildMeta = { hash: Math.random() };, it change module hash, but you have main compilation (compilation), just update hash based on source content of used files.

The main idea - if something was changed in child compilation it should change the main hash, otherwise you will faced with a lot of bugs and problems with cached, not only dev server rely on hash, a lot of other plugins/loaders/tools do it, no hash change == problems with these packages.

@wood1986
Copy link
Contributor Author

Thanks!! Your review is very useful. Let me get back to you after 8 hours.

@wood1986
Copy link
Contributor Author

Why you not emit files using webpack API?

Do you have sample code because I do not know how to use.

Firstly as you can see you use parser.state.current.buildInfo.cacheable = false and I think you understand that is hack (I already say about cache problems above and you just), you can use it using https://github.com/webpack/webpack/blob/main/lib/optimize/ConcatenatedModule.js#L774, just add all files to your deps and you can cache.

I do not know this is a hack. I remember there was a service work plugin library was doing this. And I also expect for all files which has __TOKEN__['something'] are not cachable because any change to the parent will have different filename. If you are saying I still can add all files to fileDependencies, could you show me how to get filename inside the parser? Are you saying adding to parser.state.current.buildInfo.fileDependencies

you have main compilation (compilation), just update hash based on source content of used files.

Are you saying I can explicitly update the parent compilation hash manually? When is the right stage to update hash? If I update at compilation.hooks.processAssets, the hash has already digested and I should not be able to update hash. Right?. Do you have sample code?

@alexander-akait
Copy link
Member

If you are saying I still can add all files to fileDependencies, could you show me how to get filename inside the parser?

But you are have them inside plugin... https://github.com/wood1986/demo/blob/main/index.js#L111, just do it earlier

Are you saying I can explicitly update the parent compilation hash manually?

Yes, possible solutions:

@wood1986
Copy link
Contributor Author

Yes, possible solutions:

It does not work. Take a look the commit this branch https://github.com/wood1986/demo/commit/389e5914cfb0b2845e5307fe28b3d6197926045c
I have put console.log in compilation.hooks.chunkHash, compilation.hooks.fullHash, compilation.hooks.processAssets.tapAsync and childCompiler.runAsChild

You can see the execute order of each hook.
image

I cannot register the compilation.hook.fullHash and compilation.hooks.chunkHash inside runAsChild because it is too late.

@wood1986
Copy link
Contributor Author

To be honest, I do not think it is fixable. To get it work properly, it will be either webpack add another stage in processAssets hooks with a hash parameter in the callback for people to update hash and or webpack-dev-server take my fix

@alexander-akait
Copy link
Member

There is not problems with webpack here, you should manually update hash

@alexander-akait
Copy link
Member

I cannot register the compilation.hook.fullHash and compilation.hooks.chunkHash inside runAsChild because it is too late.

You can do it early, what is the problem do it?

@alexander-akait
Copy link
Member

Again, it is invalid fix and break a lot of non official plugins

@alexander-akait
Copy link
Member

Just create variable - let hashUpdater = "";, calculate hash of files and assign to hashUpdate, in compilation.hooks.fullHash.tap update hash using hashUpdater

@alexander-akait
Copy link
Member

alexander-akait commented Oct 27, 2021

Child compilation can change something in the main compilation and do nothing, so hash can be the same or can be changed, you should manually update hash when you change something. All these problems arise because you use hacky approach, ideally - you need multi compiler mode with https://webpack.js.org/configuration/other-options/#dependencies, so you can get web content of client and inject it in node target, but you chose the way through child compilation, it has some limitations where you have to solve the problem using low API and take care of hash/cache/dependecies/etc.

I will look at this today/tomorrow (late) and show how to solve it using your approach.

@wood1986
Copy link
Contributor Author

wood1986 commented Oct 28, 2021

you need multi compiler mode with https://webpack.js.org/configuration/other-options/#dependencies, so you can get web content of client and inject it in node target

I have not tried. Where and how to configure devServer? I do not think I should put two devServer configs in each target. So if I need to put one devServer in either web or node target, I can foresee it will be no different from my plugin approach. I mean if you make a change to the entry js file, it will not have auto reload.

Other than that, as the stats of each target are completely separated, disconnected and no relationship, how can node compilation get web compilation stats? You can say I can write the manifest to the disk and then let node compilation read this from the disk. Yes, it is doable but I do not like it. It is not dev-friendly because consumers have to specify to two different plugins for web and node target. I would say this is NO.

FYI: I have tried these 3 approaches

  1. I tried to call compilation.createHash() in the callback of runAsChild(). I see the fullHash is updated but it throws an exception
    image

  2. I also tried to add these two lines in the callback of runAsChild(). It works but it is not a native hash from webpack

parentCompilation.fullHash = parentCompilation.fullHash + childCompilation.fullHash
parentCompilation.hash = parentCompilation.fullHash
  1. I also tried to use needAdditionalSeal hooks in order to have native hash and it works but I do not like parent has to redo all the hashing and it outputs parent compilation information twice in the console. All I need is the fullHash update only and no need to redo everything.

I have a feeling if you ask me to use some low api, you put the hacky code on my side. Even if you think it is not hacky, I still do not want to maintain these codes on my side. This PR does not break existing test cases. I strongly believe webpack-dev-server is missing this case because webpack's watch can re-compile on child entry change. I do not understand why you think this is not a valid solution. Meanwhile I thought webpack-dev-server relies on invalid hooks to trigger a reload. I want to know why it does not relies invalid hooks.

I totally understand your expectation child compiler's change should update parent compiler hash. But it does not support. Although you keep saying I do it in a hacky way after processAssets stage, I do not think so because the final hash value is available at this stage based on this comment. https://github.com/webpack/webpack/blob/f0298fe46fc22eebf42eb034a9435d7c19aeddd9/lib/Compilation.js#L5180-L5184

@alexander-akait
Copy link
Member

Again, because a lot of plugins use child compilation, some of them just generate external files for other usages, we can't reload app only based on this, want to reload - update hash.

Meanwhile I thought webpack-dev-server relies on invalid hooks to trigger a reload.

Is it? Then which one should he use? It was discussed a lot of time, API solves the problems of 99%. If you don't want to do something using the official approach and accordingly designed for this I do not know how to help you, unfortunately I cannot solve the problems of everyone and everything. And I don't want to break logic for many developers. If you do not believe me, I am very sad about this. If we had the opportunity to experiment (alpha/beta/rc), then I would show you how many issues will appear after this was merged. And eventually you would do revert.

@alexander-akait
Copy link
Member

alexander-akait commented Oct 28, 2021

Anyway I will look at this deeply today (have small tasks) and help you

@wood1986
Copy link
Contributor Author

Hey @alexander-akait, may I know if you are still looking at this issue?

@alexander-akait
Copy link
Member

alexander-akait commented Oct 30, 2021

@wood1986 Yes, sorry for delay, tomorrow I will look, a little busy

@wood1986
Copy link
Contributor Author

Thanks for letting me know!! FYI, I can confirm that it was working before webpack-dev-server 4.0.0

@alexander-akait
Copy link
Member

alexander-akait commented Oct 30, 2021

@wood1986 Can you provide example for v3? Try to set hot: false too

@wood1986
Copy link
Contributor Author

wood1986 commented Oct 30, 2021

Here is https://github.com/wood1986/demo/tree/wds3

I have just found. It always has auto reload when it is a new change. However when you undo the previous saved change, it will not have auto reload. I cannot reproduce in own repository but I can reproduce it in this repository

`->` means "Save"

1st change ->                         reload
1st change -> 2nd change ->           reload
1st change -> undo 1st change ->      no reload

@alexander-akait
Copy link
Member

alexander-akait commented Nov 2, 2021

@wood1986

Here is https://github.com/wood1986/demo/tree/wds3

I have just found. It always has auto reload when it is a new change. However when you undo the previous saved change, it will not have auto reload. I cannot reproduce in own repository but I can reproduce it in this repository

I see, this related to #3794, due hot: true by default (I think we really need to fix it), i.e. try to hot, if no modules, check hash, is hash was changed, do live reload

Generally we have two problems here:

  1. hot is true by default and we need fix the bug described above
  2. need to update hash of compilation (WIP on solution for you)

@alexander-akait
Copy link
Member

Oh, I see running childCompiler.runAsChild inside compilation.hooks.processAssets is too late, we already calculate hash of compilation (main) here, so we need update it manually, just do it:

compilation.hash = `${compilation.hash}${childCompilation.hash}`;

Should fix the problem, feel free to feedback

@wood1986
Copy link
Contributor Author

wood1986 commented Nov 2, 2021

I did mention this fix

See

parentCompilation.fullHash = parentCompilation.fullHash + childCompilation.fullHash
parentCompilation.hash = parentCompilation.fullHash

but it is not a native webpack hash. And do you think if other 3rd parties will have some logic related to this field compilation.hash?

Thanks for looking it up.

@alexander-akait
Copy link
Member

@wood1986 It is valid solution, for multi compiler mode we do the same, just concat two hashes in string, we have this logic https://github.com/webpack/webpack-dev-server/blob/master/client-src/utils/reloadApp.js#L12, i.e. indexOf, so invalidation works fine

but it is not a native webpack hash. And do you think if other 3rd parties will have some logic related to this field compilation.hash?

Hash is just string and can have any length (types), so even a plugin will have logic for hash it should not broken

@wood1986
Copy link
Contributor Author

wood1986 commented Nov 4, 2021

It works. Thanks!!! I am curious the multi compiler mode implementation. Is it possible to show me the which files have hash concat?

@alexander-akait
Copy link
Member

@wood1986 All used modules, you can track it in creatHash and look at used values

@alexander-akait
Copy link
Member

@wood1986 Can we close?

@wood1986 wood1986 closed this Nov 6, 2021
@wood1986 wood1986 deleted the fix-child-compiler-live-reload branch November 6, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

live reload does not works when the change is happened on entry of child compiler
2 participants