Skip to content

chore(ci): when generating patches, keep package.json from latest npm version #4344

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

Merged
merged 1 commit into from
Oct 1, 2020

Conversation

davidgovea
Copy link
Contributor

Related issues

Fixes #4331

Description

👋 Hi @mikehardy!
The patch-package failure was being caused by incomplete npm publish tasks. (repo versions higher than npm versions) (Related: #4283)
Looks like #4330 did not fix the issue

I've detailed that in a new issue #4343, and I suppose if that is fixed, then #4331 becomes a non-issue, but still, I've taken a self-contained approach based on the following assumptions:

  1. The generated patches should always be for the latest npm version, regardless of repo state.
  2. patch-package does not work well with package.json, so we can safely ignore package.json changes in the patches.

In the patch generation task, the package.json from the "live" version is now preserved.

Release Summary

N/A

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android N/A
    • iOS N/A
    • CI onlu
  • My change includes tests;
    • N/A
  • I have updated TypeScript types that are affected by my change. N/A
  • This is a breaking change;
    • Yes
    • No

Test Plan

I made a thrash-branch in my fork, adding bogus changes to each package to test the package generation.
It's very messy, but here are 2 Action runs:

@vercel
Copy link

vercel bot commented Oct 1, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/ddwya8ul4
✅ Preview: https://react-native-firebase-git-fork-davidgovea-patch-latest-version.invertase.vercel.app

@mikehardy
Copy link
Collaborator

I was literally just going to sit down and look at this with the idea the publish issue broke it, and here it appears you've done it ahead of me, you have my sincere gratitude as that leaves a lot more time for other things, like I think I'll have enough time to finish the latest stable release of AnkiDroid, so you're indirectly helping 1.7M people become doctors or fluent in a second language :). Most excellent

By the way - my hope for this approach was to package it up as an action to make it generally available for react-native projects so any PR got a patch-package. If you have any interest in becoming "Patch-Package Action Person" you have my blessing and best wishes, otherwise I'll keep it on my backlog. Just needs generalization to handle monorepos and non-monorepos

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Nice surgical fix, I like the underlying reasoning as well. Thanks

@mikehardy mikehardy merged commit 65e6255 into invertase:master Oct 1, 2020
@mikehardy
Copy link
Collaborator

the only downside to this one - a rare thing, but... - if there is a necessary change to package.json, like a new devDependency or something - no patch is generated

@davidgovea
Copy link
Contributor Author

@mikehardy I've never been able to apply a patch to package.json using patch-package successfully. I noted this in assumption 2.

There are workarounds to --exclude <nothing> in the patch creation, to side-step patch-package's default exclusion of package.json.. some related issues:
ds300/patch-package#250
ds300/patch-package#179

npm mangles the package.jsons of installed dependencies. Perhaps yarn does not? That could explain the inconsistent issue reports. If that were the case, then npm-users still could not use the generated patches.

@mikehardy
Copy link
Collaborator

Oh interesting - thanks for the extra info there. I'd say "no change" here then, and we'll just treat it as a known thing. I don't imagine most user-generated PRs will change those, and my biggest goal with the patch generation was that users could post a PR and with no effort from maintainers other users could easily test - thus reflecting the testing need back to the community but in an empowered way instead of just begging. The current capability serves that fine

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.

CI patch-package task failing to generate full set of patches
2 participants