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.

Sign up for GitHub

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

Jump to bottom

Isaacs/update libnpmetc stuff #968

Closed
wants to merge 0 commits into from
Closed

Isaacs/update libnpmetc stuff #968

wants to merge 0 commits into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Mar 3, 2020

Refactor of mikemimik/update-libnpmhook branch from pairing earlier today. Removes a lot of figgy-pudding and blue birds.

Commands updated with a new lib:

  • hook
  • access
  • team
  • org
  • search

Other random stuff:

  • cache.js
  • deprecate.js
  • fund.js
  • logout.js
  • owner.js
  • ping.js
  • star.js
  • stars.js
  • version.js
  • view.js
  • whoami.js

Utils:

  • utils/otplease.js
  • utils/git.js
  • utils/pulse-till-done.js
  • utils/read-user-info.js

Still todo:

  • publish (libnpmpublish)
  • unpublish (also libnpmpublish)
  • pack (should leverage pacote more than it does)
  • audit (should use @npmcli/arborist)
  • shrinkwrap (should use @npmcli/arborist)
  • update (should use @npmcli/arborist)
  • outdated (should use @npmcli/arborist)
  • ci (should use @npmcli/arborist)

@isaacs isaacs requested a review from a team as a code owner March 3, 2020 08:12
@mikemimik mikemimik added this to the OSS - Sprint 5 milestone Mar 3, 2020
@mikemimik mikemimik added Enhancement new feature or improvement Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes labels Mar 3, 2020
@mikemimik mikemimik linked an issue Mar 3, 2020 that may be closed by this pull request
Update libnpm* packages in the cli #970
Closed
6 tasks
@mikemimik mikemimik self-assigned this Mar 3, 2020
@isaacs
Copy link
Contributor Author

isaacs commented Mar 3, 2020

Note: this closes #935 as well.

@mikemimik mikemimik force-pushed the isaacs/update-libnpmetc-stuff branch 3 times, most recently from c6d224f to 924d85b Compare March 5, 2020 03:52
mikemimik
mikemimik suggested changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@mikemimik mikemimik left a comment

Choose a reason for hiding this comment

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

Some discussion points, @isaacs let me know if you want me to handle any of this minutia.

lib/view.js Outdated Show resolved Hide resolved
lib/version.js Outdated Show resolved Hide resolved
lib/utils/otplease.js Outdated
const readUserInfo = require('./read-user-info.js')

const isOtpError = err =>
err.code === 'EOTP' || err.code === 'E401' && /one-time pass/.test(err.body)
Copy link
Contributor

@mikemimik mikemimik Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Silly question, does this need brackets to evaluate correctly?

Suggested change
err.code === 'EOTP' || err.code === 'E401' && /one-time pass/.test(err.body)
err.code === 'EOTP' || (err.code === 'E401' && /one-time pass/.test(err.body))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not. && has higher priority than ||. In most expressions of boolean calculus, you can think of and (or any intersection operator) like multiplication or division, and or (or any union operator) like addition or subtraction. So a || b && c || d is like a + b * c + d, so it's like a + (b * c) + d and thus a || (b && c) || d.

lib/star.js Outdated Show resolved Hide resolved
lib/team.js Outdated Show resolved Hide resolved
lib/owner.js Outdated Show resolved Hide resolved
lib/config/flat-options.js Outdated
@@ -182,7 +183,6 @@ const flatOptions = npm => npm.flatOptions || Object.freeze({
strictSSL: npm.config.get('strict-ssl'),
defaultTag: npm.config.get('tag'),
get tag () {
log.warn('FIXME', 'using tag option, should be defaultTag')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I thought we wanted this? At least until we have more stability in npm@7?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that it warns every time we do ...npm.flatOptions, which we do a lot of times. And we can't make the option non-enumerable, or else it won't be carried over in those cases. We'd have to do a custom clone function that does Object.getOwnPropertyDescriptors and turns the setter into a setter on the resulting object, and then use that everywhere, but then our deps also do ...opts all over the place, so actually making that change would be almost as bad as figgy-pudding.

It was a reasonable idea, but doesn't actually work, turns out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally makes sense, I hadn't realised that. Completely agree we should just pull this out for now. Might be worth just commenting it, so we remember where our warnings were?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, if you wanna, go for it. I was just gonna leave it as a pre-v7 todo item to remove the getters from here and grep for any usage of those field names.

lib/config/flat-options.js Outdated Show resolved Hide resolved
lib/config/flat-options.js Outdated Show resolved Hide resolved
lib/config/flat-options.js Outdated Show resolved Hide resolved
isaacs
isaacs commented Mar 5, 2020
View reviewed changes
lib/ping.js Outdated Show resolved Hide resolved
@isaacs
Copy link
Contributor Author

isaacs commented Mar 5, 2020

Fixed the nits, replaced the read-json util with parse-json-even-better-errors, found 2 more bluebird .nodeify() calls that I'd overlooked.

mikemimik
mikemimik approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@mikemimik mikemimik left a comment

Choose a reason for hiding this comment

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

LGTM

lib/team.js Outdated
opts = TeamConfig(opts).concat({description: null})
// XXX: "description" option to libnpmteam is used as a description of the
// team, but in npm's options, this is a boolean meaning "show the
// description in npm search output". Hence its being set to null here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important, but eventually we can remove this.

isaacs added a commit that referenced this pull request Mar 6, 2020
@isaacs
Remove two remaining BB remnants
0bac7ed 
PR-URL: #968
Credit: @isaacs and @mikemimik
Close: #968
isaacs added a commit that referenced this pull request Mar 7, 2020
@isaacs
Remove two remaining BB remnants
bf3370b 
PR-URL: #968
Credit: @isaacs and @mikemimik
Close: #968
@isaacs isaacs closed this Mar 9, 2020
@isaacs isaacs force-pushed the isaacs/update-libnpmetc-stuff branch from 53b67a1 to bf3370b Compare March 9, 2020 16:41
isaacs added a commit that referenced this pull request May 8, 2020
@isaacs
Remove two remaining BB remnants
7df93b7 
PR-URL: #968
Credit: @isaacs and @mikemimik
Close: #968
@isaacs isaacs deleted the isaacs/update-libnpmetc-stuff branch October 2, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikemimik mikemimik mikemimik approved these changes

Assignees

@mikemimik mikemimik

Labels
Enhancement new feature or improvement Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Milestone
OSS - Sprint 5
Development

Successfully merging this pull request may close these issues.

Update libnpm* packages in the cli
2 participants
@isaacs @mikemimik