fix(npm) pass npm context everywhere#2772
Merged
ruyadorno merged 1 commit intorelease/v7.6.1from Mar 4, 2021
Merged
Conversation
Member
Author
|
This is built off of #2759 but that one should be long since landed by the time this is done. |
wraithgar
force-pushed
the
gar/npm-state
branch
3 times, most recently
from
b879719 to
333d704
Compare
isaacs
reviewed
Feb 25, 2021
isaacs
reviewed
Feb 25, 2021
isaacs
reviewed
Feb 25, 2021
|
|
||
| if (!cmd) | ||
| throw UsageError('Subcommand is required.') | ||
| exec (args, cb) { |
Contributor
There was a problem hiding this comment.
It looks like you're heading in this direction already, but pretty please can we ditch the callbacks entirely? 😆 I'd love to see this.access just be this.exec(), and have npm know how to handle that.
Member
Author
There was a problem hiding this comment.
That change was left out of this PR because of the amount of testing changes it would require. This PR is mostly code change, with minimal test changes.
Making this function async would be a minimal code change, and a larger test change. It'd be much easier than THIS PR, but still needs to be its own thing.
wraithgar
force-pushed
the
gar/npm-state
branch
15 times, most recently
from
00ccefd to
876a231
Compare
wraithgar
force-pushed
the
gar/npm-state
branch
3 times, most recently
from
7dcc94f to
5bd0d30
Compare
wraithgar
force-pushed
the
gar/npm-state
branch
2 times, most recently
from
c3b9d27 to
cbea9bc
Compare
This was referenced Mar 12, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of files randomly requiring the npm singleton, we pass it where it needs to go so that tests don't need to do so much require mocking everywhere.
Instead of continuing down the path of "Just add a function that wraps another function and adds something new to its context," commands are now proper objects.
Usage output is more correct now, that bugfix had to be in here to get things to work.
open-url had a bug where it was never handling its happy-path callback. That bug went away due to refactoring.
Tests were also added that were missing for install-ci-test and install-test.
Other minor bugfixes like how open-url was not ever handling its happy-path callback had to be fixed.
We are going to have to find a better solution for coverage on the usage and completion functions, when they are accessed through
npm.commands.xthey do not count for coverage, and that is the only way we should ultimately be trying to access them once we have proper tests.Fixes: npm/statusboard#124