Remove usage of removed API in rebuildModule #11433
Conversation
|
|
|
For maintainers only:
|
| const module = Array.from(compilation.modules).find(matcher); | ||
| /** @type {any} */ | ||
| const inputFileSystem = compilation.inputFileSystem; | ||
| const cachedFileInput = inputFileSystem._readFileBackend._data.get(src); | ||
| cachedFileInput.result = `module.exports = { foo: { foo: 'bar' }, doThings: () => { }}`; |
There was a problem hiding this comment.
Not a fan of the way in which this has to break type-safety, and dive into internal\private methods to achieve this test.
But by doing this I can test that the module has been rebuilt.
As mentioned in the description, one idea was to change the contents of the file in a second run through the use of a loader, but I'm not super comfortable with my ability to write a custom loader.
| if (originalModule) { | ||
| this.moduleGraph.removeConnection(dep); | ||
|
|
||
| if (this.chunkGraph) { |
There was a problem hiding this comment.
Noticed that chunkGraph is only created in the seal phase, and that finishModules is called before that, hence the if.
Not sure if this is entirely right however.
samcooke98
force-pushed
the
samc/fix-module-rebuild
branch
from
85e1add
to
732d15e
Compare
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
samcooke98
force-pushed
the
samc/fix-module-rebuild
branch
from
732d15e
to
fde9b52
Compare
|
It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff). A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future). @samcooke98 Please check if this is appliable to your PR and if you can add more test cases. Read the test readme for details how to write test cases. |
|
Sorry for the ping @sokra , but do you mind please giving me some pointers on how to improve the tests here, and get the coverage up? |
sokra
force-pushed
the
samc/fix-module-rebuild
branch
from
fde9b52
to
164443c
Compare
|
Thanks |
This PR is to address the issue raised here: #11269 - The
removeReasonsOfDependencyBlockmethod was calling the removeddependency.moduleWhat kind of change does this PR introduce?
Bugfix.
Did you add tests for your changes?
Yes. I'm not very happy with the method of the testing here, but I'm not familiar enough with webpack to do it in a better way.
The test works by replacing the contents of the
other-file, and change the exports. The test then asserts that the exports have changed.One idea I had was to use a loader to change the
other-file.. However I struggled with writing a custom loader.Very open to suggestions and improvements here.
Does this PR introduce a breaking change?
I don't believe so.
What needs to be documented once your changes are merged?
None, I believe?