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 rebuilding all virtual files on any change #66

Merged
merged 1 commit into from
Sep 14, 2020
Merged

Fix rebuilding all virtual files on any change #66

merged 1 commit into from
Sep 14, 2020

Conversation

non25
Copy link
Contributor

@non25 non25 commented Sep 9, 2020

What's the problem this PR addresses?

This fixes #65

How did you fix it?

Barely fixed it.
I feel like there should be a better way to implement this sort of change, but at least I've found what caused rebuilds.
It was the abscence of virtual file timestamps in compiler.fileTimestamps, causing webpack to always reload virtual files.

@larixer
Copy link
Member

larixer commented Sep 10, 2020

@non25 Thanks! How about adding the test? Otherwise your fix might be easily regressed in the future due to other changes to this plugin

@non25
Copy link
Contributor Author

non25 commented Sep 12, 2020

@larixer That took me longer than I expected.
You may congrat me with first test in my entire life I suppose.
They changed output in webpack 5, so I'm not sure if the method for getting rebuilt modules quantity is reliable in a long run.
Improved method for getting rebuilt modules quantity.
Test passes on webpack 4 & 5 at the moment.

@non25 non25 changed the title Possible fix to rebuilding every virtual file on any change Fix to rebuilding every virtual file on any change Sep 12, 2020
@non25 non25 changed the title Fix to rebuilding every virtual file on any change Fix rebuilding all virtual files on any change Sep 12, 2020
@non25
Copy link
Contributor Author

non25 commented Sep 13, 2020

It appears that only Webpack 4 needs this fix.

@larixer
Copy link
Member

larixer commented Sep 14, 2020

@non25 Thanks, looks good, though I have fears about of this test stability

@larixer larixer merged commit 0c8926e into sysgears:master Sep 14, 2020
@larixer
Copy link
Member

larixer commented Sep 14, 2020

@non25 Yeah, the test fails for me for webpack@5.0.0-beta.29, so I will have to remove the test, unfortunately

@larixer
Copy link
Member

larixer commented Sep 14, 2020

@non25 Published your other changes in webpack-virtual-modules@0.3.1

@non25
Copy link
Contributor Author

non25 commented Sep 14, 2020

That is really interesting.
It appears that they've added codeGenerated field in stats.modules only in webpack@5.0.0-beta.30... And I've wrote the test using latest webpack beta version. Until beta 30 I see no way to reliably check if module is being built. :(

Hash: eab387b1eada2df95436
Version: webpack 5.0.0-beta.29
Time: 17 ms
Built at: 2020-09-14 17:13:07
asset bundle.js 2.52 KiB [emitted] (name: bundle)
Entrypoint bundle = bundle.js
./entry.js 49 bytes [built]
./dep_one.js 8 bytes [built]
./dep_two.js 1 bytes [built]

In webpack 5 built field means that dependency is bundled, but codeGenerated field means that it is reloaded and rebuilt.

asset bundle.js 2.52 KiB [emitted] (name: bundle)
./entry.js 49 bytes [built]
./dep_one.js 8 bytes [built] [code generated]
./dep_two.js 1 bytes [built]
webpack 5.0.0-beta.30 compiled successfully in 25 ms

In webpack 4 built field means that module is being reloaded and rebuilt.

Hash: d8b06da3bdf47f40808c
Version: webpack 4.44.1
Time: 10ms
Built at: 09/14/2020 5:19:36 PM
    Asset      Size  Chunks             Chunk Names
bundle.js  4.34 KiB  bundle  [emitted]  bundle
Entrypoint bundle = bundle.js
[./dep_one.js] 8 bytes {bundle} [built]
[./dep_two.js] 0 bytes {bundle}
[./entry.js] 49 bytes {bundle}

Don't know what to do.

I feel like there's should be the test for checking are virtual modules rebuilt on any change, because that is really undesired behavior and it could pop up after some time.
There's no need to enable this fix for webpack@5.0.0-beta.30, that is for sure.

@larixer
Copy link
Member

larixer commented Sep 14, 2020

@non25 Oh, that was unexpected. Yeah, I have tried with beta.30 and the test works for me. So I have added the test back. Hopefully they won't change the behaviour in future betas

@non25
Copy link
Contributor Author

non25 commented Sep 15, 2020

@larixer Today I had an idea how to make rebuild test more robust by not trusting webpack's output on that and making loader which counts how many times it had been run...
And I found that webpack 5 actually rebuilds everything everytime.

I think test commit is much better, but I can't pull request it until I find the solution to webpack 5 rebuilds...
https://github.com/non25/webpack-virtual-modules/commits/playground

Also, I've added an example in the playground branch on how to test plugin with different webpack versions simultaneosly.
Sad part is that some webpack's dependencies like webpack terser plugin expect to import webpack at it's usual place - node_modules/webpack.
Would be nice to find a way to do webpack symlinking or node resolving using only package.json or test files.
For now this approach relies on unix ln utility and symlink-compatible fs.

@larixer
Copy link
Member

larixer commented Sep 16, 2020

@non25 There is an approach to use on Travis CI to test against multiple webpack versions we use for mochapack. Works pretty well:
https://github.com/sysgears/mochapack/blob/master/.travis.yml#L22-L28
But if using CI, I would rather use GitHub Actions now. They also have similar concept of test matrix

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.

[Bug] Virtual modules are rebuilt on any change
2 participants