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

Undo fork from #11463 #13846

Closed
JmillsExpensify opened this issue Dec 27, 2022 · 14 comments
Closed

Undo fork from #11463 #13846

JmillsExpensify opened this issue Dec 27, 2022 · 14 comments
Assignees
Labels
Improvement Item broken or needs improvement. Weekly KSv2

Comments

@JmillsExpensify
Copy link

We're forking a fork, which isn't ideal and we should undo and point upstream as soon as the PR mentioned in the HOLD is merged. I'm going to start this off as a monthly and we can re-assess from there.

Related issue: #11463

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Dec 27, 2022
@JmillsExpensify JmillsExpensify changed the title [HOLD react-native-web #10] Undo fork of [HOLD react-native-web #10] Undo fork from #11463 Dec 27, 2022
@JmillsExpensify
Copy link
Author

Still on hold and probably will be for quite some time.

@JmillsExpensify JmillsExpensify changed the title [HOLD react-native-web #10] Undo fork from #11463 Undo fork from #11463 Jan 26, 2023
@JmillsExpensify JmillsExpensify added Weekly KSv2 Improvement Item broken or needs improvement. and removed Monthly KSv2 labels Jan 26, 2023
@JmillsExpensify
Copy link
Author

Taking this issue off hold per this news.

@flodnv
Copy link
Contributor

flodnv commented Jan 27, 2023

So what do I need to do? 😂 It feels like the only good path forward is to wait for a release and merge that rnw release back into our fork @roryabraham ?

@flodnv
Copy link
Contributor

flodnv commented Jan 30, 2023

Maybe this PR resolves this issue? 😂 Expensify/react-native-web#14

@roryabraham
Copy link
Contributor

To me it doesn't look like these commits are included in Expensify/react-native-web#14.

However, I'm overall I'm confused by this issue because I suspect there are a number of custom changes we have in our react-native-web fork, and we can't go back to using the upstream until all of those changes are merged and released upstream. For example, Expensify/react-native-web#11 was just merged a few days ago and IIRC the maintainer of react-native-web had no plans to review the upstream PR until at least after RNW 0.19 is released.

@flodnv
Copy link
Contributor

flodnv commented Jan 30, 2023

To me it doesn't look like these commits are included in Expensify/react-native-web#14.

Yeah he closed the PR with another commit which is indeed included in the merge back: necolas/react-native-web@ccfd936

Yes, I have also been confused by this issue since @JmillsExpensify created it. As it stands, I think we can close it?

@Beamanator
Copy link
Contributor

FYI the upstream PR for (Expensify/react-native-web#11) is (necolas/react-native-web#2442)

  • It looks like the maintainer is actually asking about updates so maybe they're interested in merging this change upstream soon 👍

@flodnv
Copy link
Contributor

flodnv commented Jan 31, 2023

@Beamanator sorry, what does this have to do with this issue? I am so lost here 😂

@Beamanator
Copy link
Contributor

I was mainly responding to @roryabraham 's comment, sorry 😅 (that the maintainer is actually looking at the PR)

@JmillsExpensify
Copy link
Author

Per this SO, we got here through this process:

Create an E/App issue to:

Link the upstream PR while you wait for it to be merged and released: Place the issue on Weekly and prefix the title with: [Hold - Upstream -#IssueID]

Update the fork and resolve conflicts such that the PR is no longer included in the diff between the fork and the upstream

If this was the only or last PR in the fork, then update E/App to use the upstream dependency instead of the fork.

If there are still outstanding changes in the fork that have not been integrated into the upstream, then update E/App to use the updated version of the fork with the PR integrated upstream

So specifically for the last item, we created this issue, there were outstanding changes in our fork that were not integrated into the upstream. If our fork and the upstream are aligned now, then yes we can close this issue.

@roryabraham
Copy link
Contributor

If our fork and the upstream are aligned now

Not yet.

@JmillsExpensify
Copy link
Author

Ok cool, so then are we clear on why this issue exists?

@roryabraham
Copy link
Contributor

@flodnv
Copy link
Contributor

flodnv commented Feb 3, 2023

So I think we can close this given Expensify/react-native-web#14 resolves the forking. Please reopen if I'm mistaken.

@flodnv flodnv closed this as completed Feb 3, 2023
@Expensify Expensify unlocked this conversation Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

4 participants