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

Adds an auto-add-integrity option #6255

Merged
merged 6 commits into from
Aug 10, 2018
Merged

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Aug 9, 2018

Summary

This PR gives a way to disable the behavior that makes Yarn automatically add the integrity in the package.json if it is missing. This helps large codebases to adapt (they won't be blocked until the change can be done).

Note that this PR doesn't obsolete #6248 - I'll have to rebase one of them, but they both are valid on their own.

Test plan

Added a test 👍

Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

This looks good in princple, but maybe we can add a deprecation warning of some sort? Otherwise I fear we might have to forever support non-integrity lockfiles...

@arcanis
Copy link
Member Author

arcanis commented Aug 10, 2018

I don't think we have to, the switch requires an explicit acknowledgement anyway (since it defaults to true), and missing integrity isn't deprecated, since we'll likely always have to support them.

@imsnif
Copy link
Member

imsnif commented Aug 10, 2018

Regarding always having to support fields without integrity: why is that? I was thinking that a couple more PRs (adding github, urls, local files, etc.) and we can get rid of the shasum and move everything to the integrity field (similar to the way package-lock.json works). Or am I missing something?

About explicitivity (that's a word, right?): I'm just worried that people can add it to their file, forget about it and then have everything crash and burn once we (hopefully) remove shasums* from the lockfile.

*by removing shasums I just mean moving the "url suffix" we're currently using to the integrity field. I'm not discussing algorithms, sha1/sha512, etc.

@arcanis
Copy link
Member Author

arcanis commented Aug 10, 2018

Oh I see what you mean - if users don't switch now to the integrity field, then they will lose some security once we stop supporting them (it won't "crash and burn" because we would just not run the validation if it's missing, I guess). Yeah, that's true. Still, we have a lot of warning messages already, and I'm a bit afraid this one will get lost in the middle and be ignored :(

@imsnif
Copy link
Member

imsnif commented Aug 10, 2018

Makes sense... How about taking a page from the React book and calling the config something like dangerously_ignore_integrity_creation?

@arcanis
Copy link
Member Author

arcanis commented Aug 10, 2018

Good for me 👍 I'll prefix the name with unsafe-

@arcanis
Copy link
Member Author

arcanis commented Aug 10, 2018

Renamed the yarnrc field to unsafe-disable-integrity-migration

@arcanis
Copy link
Member Author

arcanis commented Aug 10, 2018

Tests are passing except for a timeout - @imsnif, all good for you?

@imsnif
Copy link
Member

imsnif commented Aug 10, 2018

Yep, looks cool!

@arcanis arcanis merged commit 36f0bf6 into yarnpkg:master Aug 10, 2018
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.

2 participants