Skip to content

Reword warning about npm to remove incorrect implications#484

Closed
forivall wants to merge 1 commit intoyarnpkg:masterfrom
forivall:reword-npm-warning
Closed

Reword warning about npm to remove incorrect implications#484
forivall wants to merge 1 commit intoyarnpkg:masterfrom
forivall:reword-npm-warning

Conversation

@forivall
Copy link
Copy Markdown
Contributor

@forivall forivall commented May 7, 2017

The warning about npm is overwhelmingly FUDdy and implies that yarn has better security than npm in their typical usage by mentioning the lack of cryptographic package signing in npm, which is also absent in yarn. also, newer versions of npm are deterministic in their installation (npm 5), and so I added a description of how installation via the OS-specific method solves that problem.

also, just in terms of phrasing, stating the conclusion first and then describing the reasons frames the statement that there's a reason why we're doing this, rather than giving a bunch of statements, and then providing the conclusion, which leaves the reader to unnecessarily construct additional conclusions relating to npm's dependability.

@Daniel15
Copy link
Copy Markdown
Member

Daniel15 commented May 7, 2017

by mentioning the lack of cryptographic package signing in npm, which is also absent in yarn

This paragraph is talking about installation of yarn itself. It's correct - Yarn is GPG signed (for the tarball, Debian and RPM packages) and Authenticode signed (for the Windows installer). npm doesn't support any of these. The fact that Yarn just checks SHA like npm does is not relevant in this section, as you never install Yarn via Yarn.

newer versions of npm are deterministic in their installation (npm 5)

npm 5 is not out of beta yet, so the current text is still accurate until a stable release of npm 5 is out. Additionally, I think we'd need to add an npm lockfile to ensure dependencies are installed deterministically, so this is still an issue. So far I haven't seen any confirmation that installing global packages via npm 5 is deterministic or that it uses the lockfile.

Have you confirmed that adding an npm lockfile results in dependencies for global packages being installed deterministically when using npm 5?

@Daniel15 Daniel15 self-requested a review May 7, 2017 21:55
@forivall
Copy link
Copy Markdown
Contributor Author

forivall commented May 7, 2017

Nevertheless, by not scoping the original assertion about to only installation of yarn causes unnecessary FUD, and leaving the reader to assume that yarn must haves solved this problem.

I haven't confirmed the deterministic nature of npm, but in my experience, for fresh installs, I haven't seen nondeterministic installs since the npm3 flattening change. I'll confirm that later.

Installation of npm with npm is deterministic because they use bundledDependencies. I'm curious why the same hasn't been considered for yarn, at least for the version pushed to npm.

@jamiebuilds
Copy link
Copy Markdown
Contributor

The paragraph appears to be correct as is, closing

@jamiebuilds jamiebuilds closed this May 9, 2017
@forivall
Copy link
Copy Markdown
Contributor Author

forivall commented May 9, 2017

@thejameskyle no, the statement about npm being non-deterministic is incorrect. It provides deterministic installs if an npm-shrinkwrap.json is present. Even better, if the bundledDependencies array is populated, then all of the files are included when the package is packaged.

@Daniel15
Copy link
Copy Markdown
Member

Daniel15 commented May 9, 2017 via email

@forivall
Copy link
Copy Markdown
Contributor Author

forivall commented May 9, 2017

Yes, I'd be open to writing a pull request that makes the installation of yarn use the features of npm that allow installation to be deterministic. However, I'd like this small fix to the documentation (which I've revised to be less substantial) to be considered first.

On points 2 and 3, i would just be able to use the shrinkwrap tests that are in the npm test suite, but substitute in the yarn generate-lock-entry and yarn.lock -> shrinkwrap conversion.

@Daniel15
Copy link
Copy Markdown
Member

Daniel15 commented May 9, 2017

However, I'd like this small fix to the documentation (which I've revised to be less substantial) to be considered first.

Your updated text looks great. Please just update "Yarn" to have an uppercase "Y" and I think it's good to go. Thank you!

@forivall
Copy link
Copy Markdown
Contributor Author

forivall commented May 9, 2017

Since this PR is closed, I've made that capitalization fix at #485. Thanks!

@gaearon
Copy link
Copy Markdown
Contributor

gaearon commented May 31, 2017

I just wanted to chime in to say we take these issues seriously, and want to avoid any kind of misrepresentation of how Yarn compares to npm.

I submitted an additional PR in #518 that I hope helps further rectify it. If you find any additional places where you feel npm behavior is misrepresented, or one could imply that, please let us know and we’ll be happy to fix those up.

Thanks! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants