Enable passthrough IPC in watch mode by znewsham · Pull Request #50890 · nodejs/node

znewsham · 2023-11-24T04:06:54Z

When spawning a node process in watch mode, from another node process, there is currently no way to get IPC between the parent and the child. This PR enables this bi-directional flow:

parent -> watcher -> child
child -> watcher -> parent

This setup is useful in situations where you have a process responsible for building and running a client/server application which needs to be made aware (via IPC) that it's client bundle has changed and needs to be re-served, without restarting the server.

debadree25 · 2023-11-24T05:03:58Z

I think this should be directed towards the main branch

znewsham · 2023-11-24T05:11:35Z

I think this should be directed towards the main branch

I confess, I'm not totally clear on the branching structure. I'd like this to be available in node 18, are you suggesting I point at v18.x or somewhere else?

debadree25 · 2023-11-24T05:14:21Z

So the general process is we land changes onto the main branch of this repo and then changes are backported to the release lines you can read about it here https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md
so for your case your branch should towards the main branch and after this change is approved and landed it shall be backported by the releasers to the 18.x branch and be available in 18.x

MoLow · 2023-11-24T11:00:45Z

please also resolve conflicts

znewsham · 2023-11-24T14:56:50Z

This will almost certainly require a backport PR

noting the original commit for my reference later: ad2131716bdd45d4279a9780f50e11e1aa4302ef

debadree25

Could you fix the first commit message according to the guidelines https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines , and these few linting issues that it complains about.

znewsham · 2023-11-25T18:21:41Z

Both of those are fixed - for some reason I can't get eslint to play nice with vscode - but the linting is now resolved.

MoLow

Changes LGTM, but I think the test can be simplified more

MoLow

LGTM

benjamingr

Nice

znewsham · 2023-12-04T16:35:46Z

Just want to check on the next steps here - the contribution guide suggests I should kick off the CI build and add an "Author Ready" label - I don't believe I have permissions to do either

nodejs-github-bot · 2023-12-04T16:42:51Z

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

znewsham · 2023-12-04T17:34:53Z

What's the best way to debug failures on specific architectures? E.g.,
node-test-commit-arm-fanned
node-test-commit-linuxone-rhel8-s390x

I could probably get myself access to an arm machine if necessary

znewsham · 2024-05-08T19:49:15Z

@MoLow I apologise, the failing test was caused by a previously fixed test that the rebase must have removed (an extraneous ' in the expected test result). Could you please re-trigger the CI run

github-actions · 2024-05-09T08:08:39Z

Failed to start CI

 ⚠ Something was pushed to the Pull Request branch since the last approving review.
✘ Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/9014337858

nodejs-github-bot · 2024-05-09T08:21:46Z

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

nodejs-github-bot · 2024-05-09T09:43:04Z

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

nodejs-github-bot · 2024-05-09T11:56:21Z

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

nodejs-github-bot · 2024-05-09T13:59:53Z

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

nodejs-github-bot · 2024-05-09T17:02:57Z

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

nodejs-github-bot · 2024-05-09T18:03:41Z

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

nodejs-github-bot · 2024-05-09T18:56:31Z

Landed in 70995bd

znewsham · 2024-05-09T19:17:02Z

@MoLow thanks for your help in getting this across the line - I've read through the contribution guide (and the backporting guide), but it still isn't clear to me which release this will appear in (or when) - will it be a minor version increase in v22, or does it have to wait until v23? Is backporting to v20 (or even v18, it's not clear what "maintenance" means) an option?

PR-URL: #50890 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MoLow · 2024-05-11T18:05:21Z

It should get to all active releases (20, 21, 22)

PR-URL: #50890 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: nodejs#50890 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. v18.x labels Nov 24, 2023

znewsham changed the base branch from v18.x-staging to mainNovember 24, 2023 14:56

znewsham force-pushed the znewsham/watch-with-ipc branch from ad21317 to 1c04659Compare November 24, 2023 14:58

debadree25 added watch-mode Issues and PRs related to watch mode and removed v18.x labels Nov 25, 2023

debadree25 reviewed Nov 25, 2023
View reviewed changes

znewsham force-pushed the znewsham/watch-with-ipc branch 2 times, most recently from 1b82c7f to 2fa9259Compare November 25, 2023 18:20

MoLow reviewed Nov 25, 2023
View reviewed changes

Comment threadlib/internal/watch_mode/files_watcher.js Outdated

MoLow reviewed Nov 25, 2023
View reviewed changes

Comment threadlib/internal/watch_mode/files_watcher.js Outdated

MoLow reviewed Nov 25, 2023
View reviewed changes

Comment threadtest/sequential/test-watch-mode.mjs

MoLow reviewed Nov 26, 2023
View reviewed changes

Comment threadlib/internal/watch_mode/files_watcher.js Outdated

MoLow reviewed Nov 26, 2023
View reviewed changes

MoLow approved these changes Nov 27, 2023
View reviewed changes

MoLow requested a review from benjamingr November 27, 2023 11:50

benjamingr reviewed Nov 27, 2023
View reviewed changes

Comment threadlib/internal/watch_mode/files_watcher.js

benjamingr approved these changes Nov 27, 2023
View reviewed changes

debadree25 added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 4, 2023

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023

watch: remove extraneous quote from failing test
2369dc8

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2024

github-actionsBot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 9, 2024

MoLow approved these changes May 9, 2024
View reviewed changes

MoLow added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels May 9, 2024

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2024

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 9, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 9, 2024

nodejs-github-bot merged commit 70995bd into nodejs:mainMay 9, 2024

targos pushed a commit that referenced this pull request May 11, 2024
watch: enable passthrough ipc in watch mode
e895f7c
PR-URL: #50890 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged

marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged

marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
watch: enable passthrough ipc in watch mode
1ff8f31
PR-URL: #50890 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
watch: enable passthrough ipc in watch mode
044678d
PR-URL: nodejs#50890 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Uh oh!

Uh oh!

Conversation

znewsham commented Nov 24, 2023

Uh oh!

debadree25 commented Nov 24, 2023

Uh oh!

znewsham commented Nov 24, 2023

Uh oh!

debadree25 commented Nov 24, 2023

Uh oh!

MoLow commented Nov 24, 2023

Uh oh!

znewsham commented Nov 24, 2023

Uh oh!

debadree25 left a comment

Choose a reason for hiding this comment

Uh oh!

znewsham commented Nov 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjamingr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

znewsham commented Dec 4, 2023

Uh oh!

nodejs-github-bot commented Dec 4, 2023

Uh oh!

znewsham commented Dec 4, 2023

Uh oh!

znewsham commented May 8, 2024

Uh oh!

github-actionsBot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

nodejs-github-bot commented May 9, 2024

Uh oh!

znewsham commented May 9, 2024

Uh oh!

MoLow commented May 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benjamingr left a comment •
edited
Loading