X Tutup
The Wayback Machine - https://web.archive.org/web/20210813053314/https://github.com/nodejs/node/pull/38862
Skip to content

/ node

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

Jump to bottom

child_process: allow options.cwd receive a URL #38862

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
Member

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

Fixes: #38861
@XadillaX
child_process: allow options.cwd receive a URL
759e5b8 
Ref: #38861
XadillaX added 2 commits May 31, 2021
@XadillaX
f
96f63c5
@XadillaX
f
de37c09
@XadillaX XadillaX requested a review from Lxxyx May 31, 2021
@aduh95
aduh95 reviewed May 31, 2021
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Can you add a change entry in the YAML in child_process.md please?
lib/child_process.js Outdated Show resolved Hide resolved
@RaisinTen
RaisinTen reviewed May 31, 2021
View changes
test/parallel/test-child-process-cwd.js Outdated Show resolved Hide resolved
@thernstig
Copy link

@thernstig 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
Member Author

@XadillaX 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 added 3 commits May 31, 2021
@XadillaX
f
6d2455f
@XadillaX
f
d3276d8
@XadillaX
f
42ea272
@XadillaX
Copy link
Member Author

@XadillaX 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 changes
doc/api/child_process.md Outdated Show resolved Hide resolved
@aduh95
aduh95 reviewed May 31, 2021
View changes
doc/api/child_process.md Outdated Show resolved Hide resolved
@aduh95
aduh95 reviewed May 31, 2021
View changes
test/parallel/test-child-process-cwd.js Outdated Show resolved Hide resolved
@jasnell
jasnell approved these changes May 31, 2021
View changes
Copy link
Member

@jasnell jasnell left a comment

LGTM with the suggested edits.
XadillaX added 2 commits Jun 1, 2021
@XadillaX
f
690dedf
@XadillaX
f
370385a
@nodejs-github-bot
Copy link

@nodejs-github-bot 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 changes
test/parallel/test-child-process-cwd.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-cwd.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-cwd.js Outdated Show resolved Hide resolved
XadillaX and others added 2 commits Jun 2, 2021
@XadillaX @aduh95
Update test/parallel/test-child-process-cwd.js
36e7dd1 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@XadillaX
f
78ad784
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Jun 2, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38446/
@XadillaX
f
3f7e729
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Jun 3, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38468/
@aduh95
aduh95 approved these changes Jun 3, 2021
View changes
XadillaX added a commit that referenced this pull request Jun 4, 2021
@XadillaX
child_process: allow options.cwd receive a URL
fc4b9c1 
PR-URL: #38862
Fixes: #38861
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@XadillaX
Copy link
Member Author

@XadillaX XadillaX commented Jun 4, 2021

Landed in fc4b9c1
@XadillaX XadillaX closed this Jun 4, 2021
targos added a commit that referenced this pull request Jun 11, 2021
@XadillaX @targos
child_process: allow options.cwd receive a URL
69fa9e1 
PR-URL: #38862
Fixes: #38861
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thernstig thernstig

@RaisinTen RaisinTen

@bl-ue bl-ue

@jasnell jasnell

@aduh95 aduh95

@Lxxyx Lxxyx

Assignees
No one assigned
Labels
child_process needs-ci semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

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
X Tutup