List of files modified during watch cycle is now accessible to plugins#9679
List of files modified during watch cycle is now accessible to plugins#9679sokra merged 18 commits intowebpack:masterfrom
Conversation
|
|
|
For maintainers only:
|
|
Hi @maxwoo-houzz. Just a little hint from a friendly bot about the best practice when submitting pull requests:
You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR. |
we don't know when the watchRun hook is called, so if we define watchRunFinished inside the hook, it could be undefined when it gets to the watchRunFinished.then(...) call so we should define it outside the hook to ensure that it is defined by the time watchRunFinished.then(...) is called also use watcher.close() callback to call done(), to ensure that it's called after the watcher is finished closed see webpack#9679 files/d5b421a408dc59ee0120019cc0947330f53fee2a#r322592231
|
@maxwoo-houzz Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
see webpack#9679 (comment) also renamed "ChangedFiles" to "ModifiedFiles" in test description removed a stray log statement
|
Looks good, I can merge it once you signed the CLA: https://cla.js.foundation/webpack/webpack?pullRequest=9679 |
|
@sokra sorry I had to double check with some people about the CLA but I just signed it! |
|
@maxwoo-houzz CLA problem fixed |
|
/cc @sokra |
we don't know when the watchRun hook is called, so if we define watchRunFinished inside the hook, it could be undefined when it gets to the watchRunFinished.then(...) call so we should define it outside the hook to ensure that it is defined by the time watchRunFinished.then(...) is called also use watcher.close() callback to call done(), to ensure that it's called after the watcher is finished closed see webpack#9679 files/d5b421a408dc59ee0120019cc0947330f53fee2a#r322592231
see webpack#9679 (comment) also renamed "ChangedFiles" to "ModifiedFiles" in test description removed a stray log statement
Thanks! :D |
CI is broken now... Working on it... |
avoid creating additional arrays by allowing Iterable on WatchFileSystem interface
… to modifiedFiles this is just merging branch 'test-expose-modified-files' into master
we don't know when the watchRun hook is called, so if we define watchRunFinished inside the hook, it could be undefined when it gets to the watchRunFinished.then(...) call so we should define it outside the hook to ensure that it is defined by the time watchRunFinished.then(...) is called also use watcher.close() callback to call done(), to ensure that it's called after the watcher is finished closed see webpack#9679 files/d5b421a408dc59ee0120019cc0947330f53fee2a#r322592231
see webpack#9679 (comment) also renamed "ChangedFiles" to "ModifiedFiles" in test description removed a stray log statement
wait a bit after creating files
|
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). @maxwoo-houzz 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. |
|
Thanks |
|
@sokra thanks for all those fixes, I couldn't really figure out what the problem was so I'm glad you got it sorted! |
The motivation behind this PR is discussed in issue #9632, but to recap, before the only way to find out which changed files triggered a re-compilation was to use WatchPack's
mtimesvariable, but this variable has been removed since WatchPack 2.0.0 (beta), so this PR provides a replacement way of getting access to this list of changed files.What kind of change does this PR introduce?
This PR adds a property to the compiler to expose the files modified during watch mode. It also fixes the test for removed files (
RemoveFiles.test.js) and adds a new testModifiedFiles.test.jsDid you add tests for your changes?
Yes, in
ModifiedFiles.test.jsDoes this PR introduce a breaking change?
No
What needs to be documented once your changes are merged?
the new compiler property
modifiedFilesshould be documented (though note thatremovedFilesisn't documented either). Also, it's important to note that currently,modifiedFilesincludes all removed files. This is due to this line of NodeWatchFileSystem, which was introduced before my PR.