Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

cross-platform shell for npm run scripts #20487

Closed
wants to merge 1 commit into from
Closed

cross-platform shell for npm run scripts #20487

wants to merge 1 commit into from

Conversation

gucong3000
Copy link
Contributor

Closes: #18580

@gucong3000 gucong3000 requested a review from a team as a code owner May 2, 2018 08:04
@iarna iarna mentioned this pull request May 4, 2018
Open
iarna
iarna suggested changes May 4, 2018
View reviewed changes
node_modules/npm-lifecycle/index.js
@@ -262,7 +262,7 @@ function runCmd_ (cmd, pkg, env, wd, opts, stage, unsafe, uid, gid, cb_) {

if (customShell) {
sh = customShell
} else if (process.platform === 'win32') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is in a dependency. To patch this you need to PR this change to npm-lifecycle.

I think this is the right approach though. When you PR this to npm-lifecycle make sure you include a default value for the isWindowsShell option that's compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@gucong3000
Copy link
Contributor Author

@iarna Is there anything else I need to do?

@zkat
Copy link
Contributor

zkat commented Jul 10, 2018

Hi! We're moving repos to https://github.com/npm/cli/pulls! See our blog post about the migration to npm.community for details. As such, we're closing all active PRs on this repo.

Could you please re-open this PR against npm/cli instead? We're still interested in this patch so we hope you will! We'll continue discussion over there once it's moved. 💚

@zkat zkat closed this Jul 10, 2018
@zkat
Copy link
Contributor

zkat commented Jul 10, 2018

p.s. You need to update npm-lifecycle in its own repo and not include the patch directly like this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iarna iarna iarna requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gucong3000 @zkat @iarna