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

Add missing npm-shrinkwrap.json #422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raphinesse
Copy link
Contributor

We had this file in Cordova 8 but must have missed it for the release of Cordova 9. As discussed before, this is important to make the release immutable.

This is a patch-level change

@dpogue
Copy link
Member

dpogue commented Apr 10, 2019

Doesn't this actually end up causing more problems than it solves though? For example, when we released a patch for cordova-lib 9.0.1, it was immediately available to everyone. With shrinkwrap we'd need to then make a new patch release of the CLI as well?

@raphinesse
Copy link
Contributor Author

raphinesse commented Apr 10, 2019

@dpogue What you say is true, we would have to do an additional release. But I think it would be preferable to know exactly which dependencies a user's Cordova installation has. I find it quite irritating. That the bugs we had in [email protected] are now fixed in [email protected]. Kind of defeats the purpose of versioning.

I don't even know what upgrade instructions I should give a user. npm i -g cordova@latest? I have no idea if that will install the latest dependencies for an already installed cordova@latest. My best guess would be that it rather does nothing at all. So we'd need to tell them to run npm un -g cordova && npm i -g cordova I guess.

@raphinesse raphinesse removed the bug label Apr 10, 2019
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

👍 The idea that 9.0.0 can contain a bug but as well have been fixed in the same version may sound confusing to an end-users.

A drawback would be the fix release turn around time.

Using the same example, if [email protected] was released with a fix, we must wait for about 1 (discuss) + 2 (vote) days before it is publicly released. Adding the shrinkwrap will now increase this 2 to 3 days release time to 5 to 6 days as we must perform the same process for cordova-cli.

Note: the discussion doesn't seem to have a hard requirement to be 1 day but the template we have gives a grace period of 1 day to talk about the release before preparing.

@raphinesse

This comment has been minimized.

@timbru31
Copy link
Member

timbru31 commented Aug 1, 2020

Do we still need this PR since we ship a package-lock.json file?

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

Adding a "visual" veto to avoid an accidental merge, since this file is outdated but has an approval

@raphinesse
Copy link
Contributor Author

raphinesse commented Aug 4, 2020

Do we still need this PR since we ship a package-lock.json file?

Yes. In fact npm-shrinkwrap.json would replace package-lock.json. In contrast to the latter, the former is distributed with packages and used when installing them. Thus it is intended for CLI packages etc. For more details see https://docs.npmjs.com/files/shrinkwrap.json

Like discussed above, this would have the benefit of actually freezing a release and the drawback of increased bugfix turnaround time, given our release process

@brodycj
Copy link
Contributor

brodycj commented Nov 25, 2020

As much as I don't like package lock & shrink-wrap files, this would probably have prevented issue #543.

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.

5 participants