-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD #333755] [$500] [MEDIUM] Distance: Create a fork for react-beautiful-dnd and remove patch #28797
Comments
FYI I will be on vacations starting tomorrow for the next two weeks and will return on Oct 23, but I will leave my thoughts here. First of all, patches are intended to be temporary fixes until you have a newer release of the library you are patching with the proper fix. In this case, we face the first problem here: The authors of the library made it clear that they won't be addressing fixes / PRs unless is a security issue.
So we can assume that this patch file has a high probability of staying here forever in our repo if we continue to use this library. Then the second problem comes, that is simply hard to make changes and manage patch files. It's difficult to understand why they are there and what they are fixing exactly. Currently we have means to separate patches fixes by using different files (e.g. the react-native patch files we have), but they need extra careful when managing and creating new ones, which increases complexity. I understand that managing a separate fork comes with an overhead to Expensify, and I'm totally fine when you are dealing with temporary situations, but this one is different by the reasons I mentioned first. |
react-beautiful-dnd
react-beautiful-dnd
Job added to Upwork: https://www.upwork.com/jobs/~01be5681d4b0e2511a |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @narefyev91 ( |
react-beautiful-dnd
ProposalPlease re-state the problem that we are trying to solve in this issue.Create a fork for react-beautiful-dnd and remove patch What is the root cause of that problem?It is a improvement What changes do you think we should make in order to solve the problem?
|
@narefyev91, @hayata-suenaga Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@hayata-suenaga this issue has |
@narefyev91, @hayata-suenaga Whoops! This issue is 2 days overdue. Let's get this updated quick! |
don't know why the title wasn't updated to put the price. updating the title |
|
Upwork job price has been updated to $500 |
ProposalPlease re-state the problem that we are trying to solve in this issue.We're using a package called react-beautiful-dnd that is no longer being maintained. To fix problems, we've created a patch and now it's decided to maintain the package ourselves. Therefore, we've forked the repo. What is the root cause of that problem?The package is no longer being maintained. It says on their page:
What changes do you think we should make in order to solve the problem?We need to apply the patch to our forked repo. We can apply the .patch file using the
We can then create a pr to the forked repo. After the pr is merged we can use the forked repo in our Expensify project. What alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.The What is the root cause of that problem?Feature Request. What changes do you think we should make in order to solve the problem?Here are the steps:We need to:
|
ProposalPlease re-state the problem that we are trying to solve in this issue.Using our own fork of react-beatiful-dnd and remove the current patch. What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
What alternative solutions did you explore? (Optional)N/A |
@hayata-suenaga Any update here? |
might have time to work on this this month |
@hayata-suenaga Any update here? |
sorry this GH ticket has the least priority in my backlog |
same as above 🙇 |
not a high priority issue now. will come back to this when I have time |
will come back to this when I have time 🙇 |
@rafecolton Any update here? |
Sorry I'm not involved in this particular issue, I just removed Hayata's assignment. Please ask in Slack. |
@DylanDylann what exactly needs to be done here? I can see we have the fork of this library, what are we waiting on? |
Waiting for @hayata-suenaga to fix this problem #28797 (comment) |
@hayata-suenaga Could you help to give an ETA on this issue? Or should we close this issue? |
@DylanDylann, Hayata is no longer at Expensify. I think we close it for focus and circle back if we have a need for it. |
@trjExpensify Yes |
Cool, done. |
@trjExpensify In this issue, I created two PRs and one of them was merged
As mentioned on Slack, the contributor should get fully paid because I am hired and did the work I am happy to hear your though. Thanks |
It has been almost a year since I started working on this issue |
Oh sorry, I thought you were settled on that. Cool, sent you an offer for $500 per the issue title. |
@trjExpensify Thank you! I accepted the offer |
Paid! |
To fix some bugs found in
react-beautiful-dnd
, we created a patch file.There was a discussion on whether we should consider creating a fork for better maintainability.
There is a strong argument that a fork should be created instead of leaving the patch file. Create a fork.
cc: @kosmydel @fabioh8010 @blazejkustra
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: