fix(npm) pass npm context everywhere by wraithgar · Pull Request #2772 · npm/cli

wraithgar · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 · 2021-02-24T23:55:45Z

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.x they 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

wraithgar · 2021-02-24T23:56:18Z

This is built off of #2759 but that one should be long since landed by the time this is done.

isaacs · 2021-02-25T16:19:35Z


- if (!cmd)
- throw UsageError('Subcommand is required.')
+ exec (args, cb) {


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.

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 3 times, most recently from b879719 to 333d704Compare February 25, 2021 15:48

isaacs reviewed Feb 25, 2021
View reviewed changes

Comment threadlib/access.js Outdated

isaacs reviewed Feb 25, 2021
View reviewed changes

Comment threadlib/access.js

isaacs reviewed Feb 25, 2021
View reviewed changes

wraithgar force-pushed the gar/npm-state branch 15 times, most recently from 00ccefd to 876a231Compare February 27, 2021 04:57

wraithgar mentioned this pull request Feb 27, 2021
Remove unused arguments on various function calls #2766
Merged

wraithgar force-pushed the gar/npm-state branch 3 times, most recently from 7dcc94f to 5bd0d30Compare February 28, 2021 04:13

wraithgar marked this pull request as ready for review February 28, 2021 04:17

wraithgar requested a review from a team as a code owner February 28, 2021 04:17

wraithgar force-pushed the gar/npm-state branch 2 times, most recently from c3b9d27 to cbea9bcCompare February 28, 2021 04:32

wraithgar deleted the gar/npm-state branch November 2, 2021 20:03