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

Isaacs/docs lockfile #2475

Closed
wants to merge 3 commits into from
Closed

Isaacs/docs lockfile #2475

wants to merge 3 commits into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Jan 12, 2021

docs for the locks


lox

References

@isaacs isaacs requested a review from a team as a code owner January 12, 2021 01:56
@@ -190,5 +190,4 @@ $ npm audit --audit-level=moderate
### See Also

* [npm install](/commands/npm-install)
* [package-locks](/configuring-npm/package-locks)
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn’t this still relevant, since npm audit still unfortunately requires a lockfile?

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 just felt somewhat out of place, especially in the "configuring-npm" section, since it's much more conceptual.

Maybe it should be brought back in (and updated for correctness) in the using-npm section?

Also, maybe Arborist.audit() should do this.loadVirtual().catch(() => this.loadActual()) or .catch(() => this.buildIdealTree()), so a lockfile isn't required? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screw the docs, yes please do that :-D

- [Checking your version of npm and Node.js](#checking-your-version-of-npm-and-node-js)
- [Using a Node version manager to install Node.js and npm](#using-a-node-version-manager-to-install-node-js-and-npm)
- [Using a Node installer to install Node.js and npm](#using-a-node-installer-to-install-node-js-and-npm)
- [Checking your version of npm and
Copy link
Collaborator

Choose a reason for hiding this comment

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

the hard wraps make it very awkward to review and presumably edit the text, and also makes git diffs much noisier; is there a reason it doesn’t soft wrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do hard wraps everywhere else in the docs. In vim, it definitely makes it easier to edit, and gq{ will re-wrap an entire paragraph easily. I typically view markdown diffs with the "rich diff" mode, so whitespace is ignored and you can just read the rendered text more easily.

I would only be mildly opposed to moving to soft wraps (since I'd have to retrain some vim settings and muscle memory) but that's a separate conversation, and in the meantime, consistency is the way to go, imo.

@npm-deploy-user
Copy link

npm-deploy-user commented Jan 12, 2021

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
legacy-peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@7 43.222 ±0.48 26.697 ±0.51 27.844 ±2.30 27.039 ±0.38 1.961 ±0.00 1.998 ±0.03 1.414 ±0.00 10.454 ±0.05 1.368 ±0.03 3.303 ±0.21
#2475 44.096 ±0.36 26.374 ±0.10 26.844 ±0.63 25.611 ±0.70 1.957 ±0.02 2.008 ±0.01 1.393 ±0.00 10.316 ±0.05 1.411 ±0.00 3.526 ±0.70
app-medium clean lock-only cache-only cache-only
legacy-peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@7 31.467 ±0.39 19.417 ±0.71 18.590 ±0.06 18.391 ±0.21 2.005 ±0.28 1.889 ±0.02 1.304 ±0.00 6.638 ±0.21 1.281 ±0.04 2.634 ±0.04
#2475 30.828 ±0.83 18.849 ±0.09 17.968 ±0.11 17.349 ±0.48 1.876 ±0.01 1.876 ±0.02 1.308 ±0.02 6.425 ±0.07 1.316 ±0.01 2.413 ±0.13

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release labels Jan 13, 2021
@isaacs
Copy link
Contributor Author

isaacs commented Jan 14, 2021

Updated docs to be correct and consistent, that npm-shrinkwrap.json has priority over package-lock.json, once @npmcli/arborist is updated to 2.0.4.

Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

Ok the shrinkwrap stuff is consistent now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants