Skip to content
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

test and refactor lib/npm.js and bin/npm-cli.js #1506

Closed
isaacs opened this issue Jul 9, 2020 · 3 comments
Closed

test and refactor lib/npm.js and bin/npm-cli.js #1506

isaacs opened this issue Jul 9, 2020 · 3 comments
Assignees
Labels
pr: needs tests requires tests before merging Release 7.x work is associated with a specific npm 7 release

Comments

@isaacs
Copy link
Contributor

isaacs commented Jul 9, 2020

  • Remove some affordances that are no longer relevant (WScript messaging, onload-script support, npm.commandName instead of npm.commands['command-name'](), optional callback/args lists, etc.)
  • unit test up to 100% coverage
  • modernize code (make npm an instance of a class, const/let/arrow-functions, organize and clean up, etc.)
@isaacs isaacs added this to the OSS - Sprint 10 milestone Jul 9, 2020
@isaacs isaacs self-assigned this Jul 9, 2020
@isaacs
Copy link
Contributor Author

isaacs commented Jul 10, 2020

Also going to update help.js in this, since the changes to the npm object have a pretty significant impact there.

@darcyclarke darcyclarke added pr: needs tests requires tests before merging Release 7.x work is associated with a specific npm 7 release labels Jul 14, 2020
@isaacs
Copy link
Contributor Author

isaacs commented Jul 20, 2020

Note: this should land after the lib/ls.js refactor, and remove the REMOVEME commit.

@ashward
Copy link

ashward commented Mar 18, 2021

Apologies for jumping on an old issue, but I wondered why onload-script support had been removed, and whether there is another way to do achieve a similar thing (hooking into the npm script whenever it's invoked) on a per-repository basis?

The reason I ask is that my security wrapper relies on this functionality, and as it stands this doesn't work with npm 7: https://github.com/ashward/jsinstallguard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: needs tests requires tests before merging Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

No branches or pull requests

4 participants