Skip to content

child_process: allow options.cwd receive a URL #38862

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

child_process: allow options.cwd receive a URL #38862

wants to merge 11 commits into from

Conversation

@XadillaX
Copy link
Contributor

@XadillaX XadillaX commented May 31, 2021 •
edited by aduh95
Loading

Fixes: #38861

@github-actions github-actions bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels May 31, 2021
@XadillaX XadillaX requested a review from Lxxyx May 31, 2021 11:51
aduh95
aduh95 reviewed May 31, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you add a change entry in the YAML in child_process.md please?

@aduh95 aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label May 31, 2021
RaisinTen
RaisinTen reviewed May 31, 2021
View reviewed changes
@thernstig
Copy link
Contributor

thernstig commented May 31, 2021

Should the code check if the URL is a path to a file or a directory? And if it is a file, strip the file from the path?

@XadillaX
Copy link
Contributor Author

XadillaX commented May 31, 2021

Should the code check if the URL is a path to a file or a directory? And if it is a file, strip the file from the path?

I think it shouldn't. The old logic doesn't check too. It will break.

@XadillaX
Copy link
Contributor Author

XadillaX commented May 31, 2021

Can you add a change entry in the YAML in child_process.md please?

Added.

aduh95
aduh95 reviewed May 31, 2021
View reviewed changes
aduh95
aduh95 reviewed May 31, 2021
View reviewed changes
aduh95
aduh95 reviewed May 31, 2021
View reviewed changes
jasnell
jasnell approved these changes May 31, 2021
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with the suggested edits.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 1, 2021

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

aduh95
aduh95 reviewed Jun 1, 2021
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 2, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 3, 2021

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

@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 4, 2021

Landed in fc4b9c1

@XadillaX XadillaX closed this Jun 4, 2021
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
@targos targos mentioned this pull request Sep 4, 2021
Merged
@LiviaMedeiros LiviaMedeiros mentioned this pull request Aug 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RaisinTen RaisinTen RaisinTen left review comments

@jasnell jasnell jasnell approved these changes

@aduh95 aduh95 aduh95 approved these changes

@Lxxyx Lxxyx Awaiting requested review from Lxxyx

+2 more reviewers

@thernstig thernstig thernstig left review comments

@bl-ue bl-ue bl-ue left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Let child_process 'cwd' arguments accept a URL

7 participants

@XadillaX @thernstig @nodejs-github-bot @jasnell @aduh95 @RaisinTen @bl-ue