-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
fix: ignore all unrelated messages from child process #13543
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
Conversation
27fc514 to
31f060f
Compare
31f060f to
ae7d77b
Compare
|
rebased on master ping @SimenB @privatenumber, is there anything that I can do in order to move this forward? |
Sorry, just swamped at work atm 🙂 We should mirror this functionality in the worker_threads implementation |
No worries at all!
Will do 👍 |
ae7d77b to
9ea0bb3
Compare
|
Rebased on latest main, and mirrored the changes for the worker_threads implementation 🐎 |
|
I don't have a whole lot of context on the whole ESM project that @SimenB has almost entirely been handling, but being lenient on this seems dangerous to me. Unless every tool and library out there starts namespacing their IPC do be clearly distinguishable (almost like making it a standard protocol), there's risk of catching and misinterpreting others' messages. I think that the code that spawns the child (which is |
|
To prevent collision, For example, prefixing the message with the |
|
Yes that would be a way of namespacing the IPC. I wouldn't be opposed to changing Like, can you point to examples of other ecosystem tools or libraries namespacing IPC in this way? Or to examples of libraries doing child-parent communication even though they were not responsible for spawning a child? I'm just not sure that it's actually Jest using an unclean approach here, versus Jest keeping it simple as everyone does and Also cc @SimenB in case you know whether it is convention for libs to assume they can use the IPC channel |
|
I don't think a convention is necessary to move forward.
It's not owned by |
SimenB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me 👍
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This is a small part of #13521 that I felt would be easier to review and merge on its own. It makes sure that any other part of the code that also uses IPC (e.g. a Node.js loader) doesn't interfere and crash the Jest process.
Test plan
I've tested this by running yarn build and manually coping the changed files into another small test project 🙈