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

feat(child_process): add node:child_process polyfill #1424

Merged
merged 15 commits into from
Nov 6, 2022

Conversation

ThatOneBro
Copy link
Contributor

Beginning of node:child_process polyfill, adds base case for spawn without any extra options specified, and the necessary functionality of the ChildProcess class.

@github-actions github-actions bot added packages:bun needs tests Something that needs more testing labels Oct 29, 2022
@Jarred-Sumner
Copy link
Collaborator

Great start!

src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
@ThatOneBro ThatOneBro changed the title feat(child_process): beginning of child_process, add ChildProcess and spawn feat(child_process): add node:child_process polyfill Nov 3, 2022
@ThatOneBro
Copy link
Contributor Author

Most of spawn parameters are now implemented, now porting all node tests to make sure everything is actually to spec. spawnSync is next which should be mostly similar except the interfaces (and the fact that it returns synchronously, obviously). The rest of the exports from child_process are just different flavored wrappers around spawn and spawnSync.

However after this, we will still lack IPC functionality. This is because we still need to add a check for a NODE_CHANNEL_FD env var and conditionally call to _forkSync at the beginning of every Bun process after spawning. This opens the channel on the child process side which is required for the two way communication.

Many of the important use cases of child_process can probably operate without IPC so we might want to merge this branch after all packages but fork are working. If that makes sense to do so.

I will give another update later after I get the node tests running and finish up spawnSync and the rest of the easy to finish exports. 👍

src/bun.js/streams.exports.js Outdated Show resolved Hide resolved
src/bun.js/streams.exports.js Outdated Show resolved Hide resolved
src/bun.js/streams.exports.js Outdated Show resolved Hide resolved
src/bun.js/streams.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
src/bun.js/child_process.exports.js Outdated Show resolved Hide resolved
});

this.stdio = this.#getBunSpawnIo(bunStdio, options);
this.stdin = this.stdio[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

these might need to be getters so that the values are the same/not copied but i'm not 100% sure

});

this.stdio = this.#getBunSpawnIo(bunStdio, options);
this.stdin = this.stdio[0];
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner Nov 6, 2022

Choose a reason for hiding this comment

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

these might need to be getters so that the values are in sync if the user assigns it but i'm not 100% sure

@Jarred-Sumner
Copy link
Collaborator

I'm going to go ahead and merge this! Great work.

If the spawnSync issue isn't fixed we can fix it in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Something that needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsupported core modules are causing errors instead of returning empty objects
2 participants