FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

watch mode: use recursive fs.watch by MoLow · Pull Request #45271 · nodejs/node · GitHub

/ node Public

watch mode: use recursive fs.watch - #45271

Closed
MoLow wants to merge 1 commit into
nodejs:mainfrom
MoLow:watch-mode-recursive-follow-up
ConversationCommits (1)ChecksFiles changed
Closed

watch mode: use recursive fs.watch#45271
MoLow wants to merge 1 commit into
nodejs:mainfrom
MoLow:watch-mode-recursive-follow-up

Conversation

MoLow commented Nov 1, 2022

Copy link
Copy Markdown
Member

follow up for #45098
adapting recursive file watching into watch mode

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Nov 1, 2022

MoLow commented Nov 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

CC @anonrig

anonrig approved these changes Nov 1, 2022
View reviewed changes

anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2022

anonrig commented Nov 1, 2022

Copy link
Copy Markdown
Member

CC @nodejs/fs

anonrig added the fs Issues and PRs related to the fs subsystem / file system. label Nov 1, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2022

nodejs-github-bot commented Nov 1, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47622/

bricss mentioned this pull request Nov 2, 2022
Merged
MoLow force-pushed the watch-mode-recursive-follow-up branch from b5f62bb to 5dbf508 Compare November 3, 2022 10:46
MoLow added the watch-mode Issues and PRs related to watch mode label Nov 3, 2022

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
Member Author

@anonrig do you have any idea why this might crash the CI?

anonrig requested changes Nov 5, 2022
View reviewed changes

anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I mostly think that due to the async nature of fs.watch is causing the errors to fail. Please add a timeout after listening

Comment thread test/parallel/test-watch-mode-files_watcher.mjs Outdated
Comment thread test/parallel/test-watch-mode-files_watcher.mjs Outdated
MoLow force-pushed the watch-mode-recursive-follow-up branch from 5dbf508 to 75f4a97 Compare November 5, 2022 22:04

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
Member Author

@anonrig I have implemented your suggestions, but it is not just a test failure - the build seems to crash

MoLow mentioned this pull request Nov 6, 2022
Merged
MoLow force-pushed the watch-mode-recursive-follow-up branch from 75f4a97 to 1699f00 Compare November 6, 2022 21:38

anonrig commented Nov 7, 2022

Copy link
Copy Markdown
Member

I think you need to skip the tests for only AIX and IBMi.

MoLow commented Nov 7, 2022

Copy link
Copy Markdown
Member Author

I think you need to skip the tests for only AIX and IBMi.

I am just trying to figure out why the build is crashing

MoLow force-pushed the watch-mode-recursive-follow-up branch 2 times, most recently from c0fefbe to fa63c9b Compare November 9, 2022 14:42
MoLow force-pushed the watch-mode-recursive-follow-up branch from 170cade to 2d6f225 Compare November 12, 2022 19:27
anonrig reviewed Nov 12, 2022
View reviewed changes
Comment thread lib/internal/fs/recursive_watch.js Outdated
MoLow force-pushed the watch-mode-recursive-follow-up branch from 2d6f225 to 7b63ee7 Compare November 13, 2022 06:36

MoLow commented Nov 13, 2022

Copy link
Copy Markdown
Member Author

will handle this after #45214 since they conflict as well

MoLow force-pushed the watch-mode-recursive-follow-up branch from 6812665 to dc77925 Compare November 13, 2022 21:41
anonrig requested changes Nov 13, 2022
View reviewed changes

anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I believe that the issue with lib/internal/fs/recursive_watch.js should be committed separately in a different pull request. Other than that, I've added some comments. Thank you for your contribution!

Comment thread test/parallel/test-fs-watch-recursive.js Outdated
Comment thread test/parallel/test-fs-watch-recursive.js Outdated
Comment thread test/parallel/test-watch-mode-files_watcher.mjs Outdated
Comment thread lib/internal/fs/recursive_watch.js Outdated
Comment thread lib/internal/fs/recursive_watch.js Outdated
MoLow force-pushed the watch-mode-recursive-follow-up branch from dc77925 to 93532f3 Compare November 17, 2022 20:20
MoLow force-pushed the watch-mode-recursive-follow-up branch from 93532f3 to 03e801d Compare November 27, 2022 22:12
MoLow mentioned this pull request May 8, 2024
Closed
MoLow closed this May 24, 2024
MoLow deleted the watch-mode-recursive-follow-up branch May 24, 2024 09:02
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anonrig anonrig requested changes

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. watch-mode Issues and PRs related to watch mode

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL