-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Implement multi-patch support in patch-package #20770
Comments
Current assignee @mallenexpensify is eligible for the NewFeature assigner, not assigning anyone new. |
@shannonmccallexfy I'm assigning you here so that you can help make sure this project gets counted as CAP SW. I guess the |
Hi! 👋🏼 I'm David Sheldrick, the author of I had a call with @roryabraham a couple of days ago to hash out a prospective scope for this feature. He asked me to write up the following proposal: ScopeThe project can broadly be broken down into three core features:
All features will have appropriate unit and integration tests, in line with the existing patch-package codebase. Integration with existing patch-package featuresThe The Budget allocationThe total budget for this project is $20,000, and I'd propose allocating it between two milestones as follows
This would include a two-month period of asynchronous support + bug fixing while you battle-test the features, starting on the day of final delivery of milestone 2. I can also provide up to 2 hours of synchronous zoom support in the EST time zone over the same two-month period if needed, scheduled ad-hoc and subject to availability. TimelineI expect to work on this in 1–2-hour chunks over the period of a month or so. I'll be working on it in my spare time during the height of summer so I can't commit to a fixed timeline, but I believe it's a realistic estimate. I'm obviously happy to keep you updated on my progress on a regular schedule (e.g. on Mondays and Thursdays, or whatever works for you), and I'll do the work in the open on the patch-package repo so you can follow along. Payment methodIf convenient you can pay me via one-time GitHub sponsors donations, which I already have set up on the patch-package repo. Otherwise Rory mentioned that you normally use Upwork, which I am happy to register for. Thanks, |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@ds300 What if you want to edit a specific patch file rather than the latest one? Maybe there should be three options:
The use-case for that might be that you introduced a patch associated with an upstream PR, but then you make changes to the upstream and want to apply the same changes to your patch. Rather than creating a second patch associated with the same upstream PR, you would update the existing one.
👍🏼 to both of the items there. |
Overall, super excited for this feature, and thrilled that you're ready to build it for us and the whole community. I've assigned this issue to you, which means that we agree to your proposed scope and budget and you can start work whenever you're ready. Thanks! 🙇🏼 |
First Monday check-in! 🕐 I haven't broken ground yet, planning to get started tonight after dinner.
I think this would have to be part of the rebase tool, in order to make sure that it works for patches that build on one another sequentially. Just like git's rebase it could target a particular patch file in the 'history' and roll back to that point.
When you're done |
The work is happening in this PR ds300/patch-package#474 |
Thanks for the help with this @ds300 , I'll be managing payment. Our normal process is to issue payment 7 days after PRs hit production, to ensure their aren't any regressions. I'm unsure if that makes sense here though. Once we get closer to completion we'll discuss payment options, it'd be great to minimize any fees you might have to pay. |
Thursday check-in! 🕐 I have finished an initial implementation of patch sequence application and have done some testing for it. It seems to work well but in order to make it usable for patch generation and rebase it will need to be refactored to decouple the patch application bits from the error handling and console logging bits. I plan to get started on that on Saturday morning. |
Monday check-in! 🕐 I got an initial implementation of patch sequence generation working. I need to improve the error reporting a little and do some more integration testing for it. After that I'll do some QA on different OSes, node versions, and package managers. After which I'll merge this PR and ping you. Probably some time later this week. Then I'll get started on the rebase feature. I published a canary release of this branch on npm. You can do |
No changes since Monday. I expect i'll be able to work on it next on Monday |
Monday check-in! 🕐 I did some manual testing on all three platforms, then added and tested some sanity checks. I published a new canary and will merge the PR soon to begin work on the --rebase feature. |
Nothing to report today. I'm planning to get started on --rebase on Saturday morning. |
Thanks for the consistent updates @ds300! |
Monday update 🕐 ! tldr; ran into some unexpected complexity with patch sequence application. It is resolved now. While sitting down on Saturday to review the current state of the work and plan the --rebase feature I noticed that the new sequenced patch application code was no longer idempotent, and indeed would fail on subsequent attempts if patches build on top of one another in overlapping regions of code. The patch application idempotency has so far been achieved by the following algorithm:
With a sequence of patches, doing it in this fashion (i.e. trying to 'dryly' apply the sequence backwards) would introduce a good bit of extra complexity. Also, while thinking about the --rebase feature I realised it would require storing some state on disk the same way git does. So I opted to store a 'state' file already, in the patched package directory, e.g. We still use the old algo above if there is only one patch to apply, since it is battle-tested and naturally more robust due to being stateless. I just finished this off and am now planning to get started on --rebase in earnest tomorrow. I have the day off on Wednesday and I'm planning to get the feature alpha testable by the end of the week. I also just published a new canary with the idempotency fix. |
FWIW it looks like we are going to take a stab at using this feature tomorrow. So @fabioh8010 if you have any feedback on the feature feel free to leave it here. |
Hi 👋 I was able to use the |
@fabioh8010 that's great news! 🎉
It's currently possible to edit only the last patch, by making changes inside node_modules and running The 🕐 I have nearly finished implementing an initial version of |
Thanks for your patience @ds300, we're discussing the best way to pay you. |
@ds300 the plan is:
That sound good? I should be able to handle this week, I'm waiting for my card limit to be raised |
Works for me, thanks @mallenexpensify 🙏🏼 |
@ds300 , I've checked with out accounting team and we need to pay you through Expensify. Contributor: @ds300 is due $20,000 via Expensify NewDot. @JmillsExpensify and/or @anmurali can you please issue payment? What are the steps @ds300 needs to do in order to receive payment, it looks like he's in the UK. |
@mallenexpensify Can you add him to the Contributor Payments workspace owned by [email protected]? Then he'll need to |
Thanks @JmillsExpensify I've added @ds300 via their gmail address we've communicated through before. I send these steps, let me know if anything doesn't look correct
Anyone know what the best payment setup is for someone in the UK? Wise? Can we do direct transfer/payment to a bank account there? |
Posted question in slack here |
They can add their UK bank account or a Wise account, it's their call. |
$20,000 payment approved via NewDot based on BZ summary. |
I confirm |
Thanks @ds300. Can you comment once you've received the payment then I'll close this. |
@ds300, @mallenexpensify, @roryabraham, @shannonmccallexfy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
I just received a notification saying that an ACH direct deposit was made, and I should have the money by Oct 4th. Will post an update here once it's gone through. |
The payment went through! Thanks for figuring this out @mallenexpensify and @roryabraham. It's been a pleasure to work with you both. |
Great news, thanks for building this awesome feature for us. I hope the open-source community finds it as valuable as we do! |
@mallenexpensify @roryabraham @shannonmccallexfy Be sure to fill out the Contact List! |
Thanks @ds300 , glad everything worked out! |
Problem
patch-package is a simple and useful package that we use to implement bug fixes and new features in 3rd-party libraries without creating a fork. However, it's limited in that each package can have only one monolithic patch file. This makes it difficult to keep track of separate changes in a 3rd-party package, which is one of the reasons we've opted for forks in the past. You can track pull requests in the fork in parallel with their upstream equivalent and use that to reconcile all the custom changes we've made to a 3rd-party dependency. This also helps indicate when our code has landed upstream and the fork/patch is no longer needed.
Solution
Upgrade patch-package to support multiple patch files per dependency. To ground this with an example, let's say we wanted to patch an image bug in React Native, and also wanted to build a new feature in the TextInput component.
For each of these features, we could have a separate patch file for each patch, and those patches would be applied in sequence:
We could have a separate PR in E/App to introduce each of these patches, and those PRs would link to their upstream equivalent.
Furthermore, we should have error reporting on a per-patch basis, and a way to resolve conflicts in specific patches while upgrading the base version.
Additional context
The text was updated successfully, but these errors were encountered: