Skip to content

watch mode: use recursive fs.watch - #45271

Closed
MoLow wants to merge 1 commit into
nodejs:mainfrom
MoLow:watch-mode-recursive-follow-up
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

@MoLowMoLow commented Nov 1, 2022

Copy link
Copy Markdown
Member

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

@nodejs-github-botnodejs-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

MoLow commented Nov 1, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

CC @anonrig

anonrig
anonrig approved these changes Nov 1, 2022
View reviewed changes

@anonriganonrig 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.

lgtm

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

anonrig commented Nov 1, 2022

Copy link
Copy Markdown
Member

CC @nodejs/fs

@anonriganonrig added the fs Issues and PRs related to the fs subsystem / file system. label Nov 1, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 1, 2022

Copy link
Copy Markdown
Collaborator

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

@bricssbricss mentioned this pull request Nov 2, 2022
Merged
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from b5f62bb to 5dbf508CompareNovember 3, 2022 10:46
@MoLowMoLow added the watch-mode Issues and PRs related to watch mode label Nov 3, 2022
@MoLow

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
MemberAuthor

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

anonrig
anonrig requested changes Nov 5, 2022
View reviewed changes

@anonriganonrig 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.

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 threadtest/parallel/test-watch-mode-files_watcher.mjs Outdated
Comment threadtest/parallel/test-watch-mode-files_watcher.mjs Outdated
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from 5dbf508 to 75f4a97CompareNovember 5, 2022 22:04
@MoLow

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
MemberAuthor

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

@MoLowMoLow mentioned this pull request Nov 6, 2022
Merged
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from 75f4a97 to 1699f00CompareNovember 6, 2022 21:38
@anonrig

anonrig commented Nov 7, 2022

Copy link
Copy Markdown
Member

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

@MoLow

MoLow commented Nov 7, 2022

Copy link
Copy Markdown
MemberAuthor

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
MoLowforce-pushed the watch-mode-recursive-follow-up branch 2 times, most recently from c0fefbe to fa63c9bCompareNovember 9, 2022 14:42
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from 170cade to 2d6f225CompareNovember 12, 2022 19:27
anonrig
anonrig reviewed Nov 12, 2022
View reviewed changes
Comment threadlib/internal/fs/recursive_watch.js Outdated
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from 2d6f225 to 7b63ee7CompareNovember 13, 2022 06:36
@MoLow

MoLow commented Nov 13, 2022

Copy link
Copy Markdown
MemberAuthor

will handle this after #45214 since they conflict as well

@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from 6812665 to dc77925CompareNovember 13, 2022 21:41
anonrig
anonrig requested changes Nov 13, 2022
View reviewed changes

@anonriganonrig 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.

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 threadtest/parallel/test-fs-watch-recursive.js Outdated
Comment threadtest/parallel/test-fs-watch-recursive.js Outdated
Comment threadtest/parallel/test-watch-mode-files_watcher.mjs Outdated
Comment threadlib/internal/fs/recursive_watch.js Outdated
Comment threadlib/internal/fs/recursive_watch.js Outdated
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from dc77925 to 93532f3CompareNovember 17, 2022 20:20
@MoLow
MoLowforce-pushed the watch-mode-recursive-follow-up branch from 93532f3 to 03e801dCompareNovember 27, 2022 22:12
@MoLowMoLow mentioned this pull request May 8, 2024
Open
@MoLowMoLow closed this May 24, 2024
@MoLow
MoLow deleted the watch-mode-recursive-follow-up branch May 24, 2024 09:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig requested changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MoLow@anonrig@nodejs-github-bot