fix: Enable cache with externalDependencies
#984
Conversation
There was a problem hiding this comment.
Thanks for the PR. I am afraid this approach is not very performant: essentially we are querying fs for external dependencies stats during every webpack re-compilation, even though most of re-run may not be trigged by external dependencies.
We can obtain a set of changed files from the webpack watch file system:
this._compiler.watchFileSystem.watcher.getAggregated()
// returns { changes: Set<string>, removals: Set<string> }and then we can see if there are changes in external dependencies, if so we can invalidate and purge the cache, otherwise we assume the cache is safe to be reused. In this approach all the extra works are done in memory, no extra IO is involved.
Of course this._compiler is a webpack specific property, the docs has warned that using this_compiler would "have a negative impact on your loaders compatibility." We can open an issue at the webpack repo to ask for their input.
Maybe in webpack5 we don't need to implement our own caching. |
|
|
Well |
Does it work if we pass absolute file paths to |
|
Should already be an absolute path in this test. |
Yes, I plan to finish the backport first to fix the regression, then migrate the code to ts, and then try to refactor the cache. |
| @@ -1 +1,4 @@ | |||
| yarnPath: .yarn/releases/yarn-3.2.3.cjs | |||
| enableGlobalCache: true | |||
| nodeLinker: node-modules | |||
There was a problem hiding this comment.
My prettier-vscode doesn't seem to be working properly.
There was a problem hiding this comment.
Did you try https://yarnpkg.com/getting-started/editor-sdks?
|
|
||
| t.true(stats.compilation.fileDependencies.has(dep)); | ||
|
|
||
| t.is(counter, 2); |
There was a problem hiding this comment.
Unfortunately this still caches twice.
There was a problem hiding this comment.
Can you retrieve the filestamps of external dependencies from
this._compilation.fileSystemInfo.getFileTimestamp(path: string, callback)
webpack will add it to the builtin file timestamp cache, if cache is not hit, it will parallelize the filesystem stat call.
I realize that the watcher approach does not cover all situation since changes on external dependencies might happen out of the webpack watch cycle. So mtime is the correct approach.
# Conflicts: # .yarnrc.yml # src/index.js
* Add tests from #984 # Conflicts: # .yarnrc.yml # src/index.js * migrate test to node test styles * feat: enable cache where there are external deps * chore: fix dead links in comments * fix lint errors * save dep and timestamp as tuple * simplify handleExternalDependencies interface * chore: create getFileTimestamp only when cache is enabled --------- Co-authored-by: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com>
Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
Fixes: #983
What is the new behavior?
Does this PR introduce a breaking change?
If this PR contains a breaking change, please describe the following...
Other information:
I'm not sure how to test it, I manually tested it successfully in the repro of #983.
This may need to be backported to 8.x as this is a regression.