lib: support --watch use with --watch-path #46000
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch is removing --watch-path=xxx from the child command line argument. The code seemed to only remove --watch-path xxx args.
This patch is removing the hardcoded clear command in the log and check for --watch-preserve-output before clearing.
This patch is adding support for --watch-path when using --watch to watch extra files/paths. When used together, the path(s) passed to --watch-path will be added to the filteredFiles in the file watcher. Fixes: nodejs#45467
This patch is adding test for the new feature.
aduh95
reviewed
Dec 29, 2022
lib/internal/main/watch_mode.js
Outdated
| const kCommand = ArrayPrototypeSlice(process.argv, 1); | ||
| const kCommandStr = inspect(ArrayPrototypeJoin(kCommand, ' ')); | ||
| const args = ArrayPrototypeFilter(process.execArgv, (arg, i, arr) => | ||
| arg !== '--watch-path' && arr[i - 1] !== '--watch-path' && arg !== '--watch' && arg !== '--watch-preserve-output'); | ||
| !arg.startsWith('--watch-path') && |
There was a problem hiding this comment.
Why use startsWith instead of !==? How is it related to this change?
Suggested change
| !arg.startsWith('--watch-path') && | |
| !StringPrototypeStartsWith(arg, '--watch-path') && |
There was a problem hiding this comment.
From what I saw, in the test it is passing the arg like --watch-path xxx but in the doc I see --watch-path=xxx and in this case --watch-path=xxx was not removed from the args passed to the child process. When that happend that caused an infite loop because the child was called like node --watch-path=xxx entry.js
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
@aduh95 Hello, any chance I can get a review on this one? thanks! |
|
@ydeshayes sorry I missed the ping, my GitHub notifications are all over the place 😵💫 Any chance you could rebase to solve the git conflict? |
|
LGTM, please rebase |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch is adding support for --watch-path when using --watch by adding the path passed to --watch-path in the filtered files instead of calling watchPath directly.
So now the app will reload if the entry point or one of the dependency change, but also if one of the --watch-path change.
The aim was to keep the current behavior of --watch-path when used alone.
It is also fixing the child arguments, now removing the --watch-path=xxx from it.
It is also fixing a hardcoded clear that did not account for --watch-preserve-output.
Fixes: #45467