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

Error after upgrade prevents package.json and yarn.lock from being updated #86

Closed
OliverJAsh opened this issue Sep 24, 2018 · 6 comments · Fixed by #112
Closed

Error after upgrade prevents package.json and yarn.lock from being updated #86

OliverJAsh opened this issue Sep 24, 2018 · 6 comments · Fixed by #112

Comments

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Sep 24, 2018

When I try to upgrade a dependency (using yarn upgrade) and I get a new patch failure error from patch-package, I'm left in a weird state where my node_modules do reflect the changes, but my package.json and yarn.lock do not.

My presumption is that the patch-package error stops Yarn from commiting the update to the manifest and lockfile. I'm not sure this is desired behaviour, or whether this is a patch-package problem or a Yarn problem.

Because of the error message, I thought it was safe to go in and re-apply my patch. So I edited node_modules and ran patch-package to generate the new patch, but because the upgrade was not committed, patch-package created a patch of a diff between the version before the upgrade and the version after the upgrade with my changes, which took me awhile to realise.

If we can't fix the bug whereby upgrades are not committed due to the error, we should provide clearer steps in the error message to avoid this.

@OliverJAsh
Copy link
Contributor Author

I just ran into this again. Any ideas how we can fix this?

@ds300
Copy link
Owner

ds300 commented Dec 13, 2018

Hi 👋 Thanks for the report!

If I understand correctly, it sounds like if patch-package returns > 0 during the 'postinstall' or 'prepare' hook then yarn takes that to mean the upgrade failed and won't touch yarn.lock or package.json but the node_modules folder still has the upgraded package?

That's interesting behaviour! I suppose in that case it is not a good idea for patch-package to return > 1 🤔 – It should just be printing an error and hope you notice it. Maybe I could add a patch-package --verify command that people can add to their test script so this kind of thing doesn't sneak past CI.

@OliverJAsh
Copy link
Contributor Author

yarn takes that to mean the upgrade failed and won't touch yarn.lock or package.json but the node_modules folder still has the upgraded package

Yes, that's correct. I wish that wasn't the behaviour of yarn 😒 Maybe we should file this as a yarn bug.

Failing that we can get this behaviour fixed in yarn, I like your idea.

@ds300
Copy link
Owner

ds300 commented Jan 20, 2019

Hi @OliverJAsh I figured out a pragmatic solution: calling exit(1) on CI and exit(0) locally. That prevents the confusing upgrade issues while also preventing bad patch files from sneaking past CI, without adding any extra commands. I'll publish a fix for this shortly.

@ds300
Copy link
Owner

ds300 commented Jan 20, 2019

image

@OliverJAsh
Copy link
Contributor Author

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 a pull request may close this issue.

3 participants
@OliverJAsh @ds300 and others