Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable passthrough IPC in watch mode #50890

Merged
merged 9 commits into from
May 9, 2024

Conversation

znewsham
Copy link
Contributor

Fixes #50880

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.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Nov 24, 2023
@debadree25
Copy link
Member

I think this should be directed towards the main branch

@znewsham
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

MoLow commented Nov 24, 2023

please also resolve conflicts

@znewsham
Copy link
Contributor Author

This will almost certainly require a backport PR

noting the original commit for my reference later: ad2131716bdd45d4279a9780f50e11e1aa4302ef

@znewsham znewsham changed the base branch from v18.x-staging to main November 24, 2023 14:56
@znewsham znewsham force-pushed the znewsham/watch-with-ipc branch from ad21317 to 1c04659 Compare November 24, 2023 14:58
@debadree25 debadree25 added watch-mode Issues and PRs related to watch mode and removed v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Nov 25, 2023
Copy link
Member

@debadree25 debadree25 left a comment

Choose a reason for hiding this comment

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

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 znewsham force-pushed the znewsham/watch-with-ipc branch 2 times, most recently from 1b82c7f to 2fa9259 Compare November 25, 2023 18:20
@znewsham
Copy link
Contributor Author

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

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

LGTM

@MoLow MoLow requested a review from benjamingr November 27, 2023 11:50
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Nice

@znewsham
Copy link
Contributor Author

znewsham commented Dec 4, 2023

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

@debadree25 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@znewsham
Copy link
Contributor Author

znewsham commented Dec 4, 2023

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
Copy link
Contributor Author

znewsham commented May 8, 2024

@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

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2024
@github-actions github-actions bot 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
Copy link
Contributor

github-actions bot commented May 9, 2024

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

@MoLow 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-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow 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 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 nodejs-github-bot merged commit 70995bd into nodejs:main May 9, 2024
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 70995bd

@znewsham
Copy link
Contributor Author

znewsham commented May 9, 2024

@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?

targos pushed a commit that referenced this pull request May 11, 2024
PR-URL: #50890
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MoLow
Copy link
Member

MoLow commented May 11, 2024

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

marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #50890
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #50890
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
PR-URL: nodejs#50890
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
PR-URL: nodejs#50890
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. watch-mode Issues and PRs related to watch mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--watch mode disables IPC communication between a spawning process and the actual child process
6 participants