-
Notifications
You must be signed in to change notification settings - Fork 634
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
feat(node): add child_process.fork #1695
Conversation
The CI is blocked by #1694 |
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.
@kt3k is this ready? I don't see any new method in the chil_process.ts
file.
@bartlomieju This should add |
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.
Thanks @kt3k I was looking at the wrong file. This API is gonna be tricky due to flags, but this is good progress
node/child_process.ts
Outdated
"--compat", | ||
"--unstable", | ||
"--no-check", | ||
"--location=https://example.com", |
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.
Why is location
argument needed?
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.
Some npm module does something like typeof location
to detect the environment and causes uncaught errors. So giving --location
avoids those errors, but that probably should be solved by denoland/deno#12969
Ok. I'll remove this option as a first pass
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.
Yeah, let's address it in Deno directly
"--unstable", | ||
"--no-check", | ||
"--location=https://example.com", | ||
"--allow-all", |
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.
Yeah... this is problematic, ideally we'd use the same permissions as the parent process, but unfortunately we don't have access to these flags.
Oh, because we use |
Unfortunately I don't see a good way to fix it at the moment, as |
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.
LGTM
env: | ||
DENO_NODE_COMPAT_URL: file:///${{ github.workspace }}/ |
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.
This is a good workaround for now
This PR implements child_process.fork in
std/node
This implements at least the basic feature of
fork
and enabled the basic test cases (child-process-spawn-node.js
).This doesn't implement
ipc
features. So I commented out ipe-related assertion in the test cases.The diff might look big, but that is because this PR moves
ChildProcess
class fromnode/child_process
tonode/internal/chil_process
to follow the internal structure of Node.js code base.closes #1690