Skip to content

fix: npm explore spawn shell correctly#784

Closed
jasisk wants to merge 1 commit intonpm:latestfrom
jasisk:js/fix-explore
Closed

fix: npm explore spawn shell correctly#784
jasisk wants to merge 1 commit intonpm:latestfrom
jasisk:js/fix-explore

Conversation

@jasisk
Copy link
Contributor

@jasisk jasisk commented Feb 7, 2020

What / Why

Fix npm explore <packagename> (with no additional arguments) shell spawning.

References

@jasisk jasisk requested a review from a team as a code owner February 7, 2020 22:36
@mikemimik mikemimik added Bug thing that needs fixing Community Enhancement new feature or improvement pr: needs tests requires tests before merging semver:patch semver patch level for changes labels Feb 13, 2020
@mikemimik
Copy link
Contributor

mikemimik commented Feb 13, 2020

Hey @jasisk thanks for contributing this!

Is it possible for you to write a failing tests for this bug, so as your introduced fix causes the tests to pass?

Let us know if you need any help :D

@mikemimik mikemimik added the Priority Backlog a "backlogged" item that will be tracked in a Project Board label Feb 13, 2020
@jasisk
Copy link
Contributor Author

jasisk commented Feb 13, 2020 •
edited
Loading

Definitely! I wandered into the tests to see if there was already a convention for testing explore specifically but couldn't find anything that looked like the right shape. Mind pointing me to an existing test I should model it off of?

edit: happy for the free-for-all if there isn't something I should reference—I just try to leave things looking like how they were before I got there. Whatever you advise. 😀

@mikemimik
Copy link
Contributor

mikemimik commented Feb 17, 2020

Hey @jasisk looks like the only place that command comes up is in test/tap/builtin-config.js. That file might be simple enough to give you some direction on how we test the cli project. Feel free to create a new file for the test you're creating for this bug, don't feel the need to clump yours in with the file I referenced.

Again, if you need any help, let us know! :D

@darcyclarke darcyclarke added this to the OSS - Sprint 11 milestone Jul 20, 2020
@isaacs
Copy link
Contributor

isaacs commented Jul 20, 2020

This LGTM for the next v6 patch release.

I'll pull into the v7 branch and add tests now, since the tests for explore are pretty lacking anyway.

@isaacs isaacs mentioned this pull request Jul 20, 2020
Closed
@jasisk
Copy link
Contributor Author

jasisk commented Jul 20, 2020

Completely forgot about this one. Thanks for pulling it in, @isaacs. Happy to take care of anything you need here.

isaacs pushed a commit that referenced this pull request Jul 20, 2020
@jasisk @isaacs
fix: npm explore spawn shell correctly
e19d1a7 
PR-URL: #784
Credit: @jasisk
Close: #784
Reviewed-by: @isaacs
isaacs pushed a commit that referenced this pull request Jul 20, 2020
@jasisk @isaacs
fix: npm explore spawn shell correctly
7d1329f 
PR-URL: #784
Credit: @jasisk
Close: #784
Reviewed-by: @isaacs
@claudiahdz claudiahdz mentioned this pull request Jul 21, 2020
Closed
@snyk-io snyk-io bot mentioned this pull request Oct 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug thing that needs fixing Enhancement new feature or improvement pr: needs tests requires tests before merging Priority Backlog a "backlogged" item that will be tracked in a Project Board semver:patch semver patch level for changes

Projects

None yet

Milestone

OSS - Sprint 11

Development

Successfully merging this pull request may close these issues.

[BUG] npm explore shell spawning without arguments immediately terminates (!isWindows)

4 participants

@jasisk @mikemimik @isaacs @darcyclarke