Follow some of RN's Flipper changes before taking RN v0.63 upgrade. #4322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following is a list of commits filtered down (with order preserved) from the output of in the RN repo. Filtered by...if it seems plausibly related to Flipper, when I read the commit message and/or the changes. 🙂
|
|
Thanks for this detailed and easy-to-follow analysis! This all looks great.
Yeah. When we do that, we'll be getting the version that's in the RN we're running. The version we're currently on doesn't have this code at all (not as a standalone, invokable script), so we can't really do this before the upgrade. Probably best is as a commit coming in the upgrade PR after the upgrade commit.
We'll effectively get this when we switch to using the script from RN, right? In that case probably cleanest to take this just as part of following along with RN's version of this code. Then when we switch over, the switch can be NFC or nearly so, which is nice. Please merge at will after that! |
…dency. Part of the RN v0.62 -> v0.63 changes to the template app [1], corresponding to facebook/react-native@a642c3f26. This seems appropriate to do right away; a comment [2] on the corresponding RN issue links (as "the culprit") to a line in v0.33.1 of Flipper [3]. We're on v0.39.0, so it seems like it's time for us to take this fix. The consequence of not making this change is reported to be an inability to develop/debug with Android API < 21. There are reportedly no effects in production. [1] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [2] facebook/react-native#28481 (comment) [3] https://github.com/facebook/flipper/blob/v0.33.1/build.gradle#L89
When React Native upgraded its main Flipper dependency for iOS to 0.36.0, in facebook/react-native@27ccc6019, it also bumped these two related dependencies. Since we've upgraded to v0.39.0 (we did that in f7005c5), we might as well follow React Native's lead. It probably would have been best to do these at the same time, but better late than never. When looking at facebook/react-native@27ccc6019, note that it comes after facebook/react-native@619d5d60d, in which React Native stopped pushing lots of Flipper configuration code out to the consumer's Podfile. So we'll eventually get to stop making changes like this one, but not until we've upgraded to React Native v0.63. That's why this commit touches our Podfile while the RN upstream commit doesn't.
…r 41. Part of the RN v0.62 -> v0.63 changes to the template app [1], corresponding to facebook/react-native@29639e7b9, plus necessary changes to the Podfile. The interesting change to the Podfile is the upgrade to Flipper v0.41.0; the s/DoubleConversion/Flipper-DoubleConversion change is NFC. Here's why this commit touches our Podfile, while the RN upstream commit doesn't: the upstream commit came after facebook/react-native@619d5d60d, in which React Native stopped pushing lots of Flipper configuration code out to the consumer's Podfile. So we'll eventually get to stop making changes like these to the Podfile, but not until we've taken the changes in React Native, which we'll do by upgrading to v0.63. facebook/react-native@29639e7b9 also has a chunk of deletions alongside the Flipper v0.41.0 upgrade. That chunk is a clean reversion of a chunk of insertions in facebook/react-native@27ccc6019 -- so we simplify by not taking the insertions in the first place. [1] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4
…ion. Part of the RN v0.62 -> v0.63 changes to the Flipper-pods logic, transposed to the Podfile, corresponding to facebook/react-native@c72ecdb90. See the last few commits for why the upstream commit makes these changes somewhere other than the template-app Podfile (and why the changes don't appear in the upgrade guide [1]). [1] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4
Part of the RN v0.62 -> v0.63 changes to the template app [1], corresponding to facebook/react-native@66c8cd393, plus a necessary change to the Podfile. See the last few commits for why we make a change to the Podfile while the RN upstream commit does not. [1] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4
Part of the RN v0.62 -> v0.63 changes to the template app [1], corresponding to facebook/react-native@4bb0b4f20. These lines (which we added in cb56f16, for facebook/react-native@2fd50882b) are reportedly "no longer necessary with the new Flipper release". It would have been nice if they specified which Flipper release. At that commit, the Flipper version in use is 0.30.2, so it probably would have been safe to make this change for quite a while -- but might as well do it now, after having just taken a big leap in Flipper versions well beyond that. [1] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [2] https://github.com/facebook/react-native/blob/4bb0b4f20/template/android/gradle.properties#L28
chrisbobbe
force-pushed
the
pr-upgrade-flipper
branch
from
86ab92d
to
6bd4274
Compare
|
Thanks for the review!
Sure, makes sense! Done.
OK, done! Thanks again for the review. |
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 20, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 20, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 20, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 21, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 21, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 22, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
Gautam-Arora24
pushed a commit
to Gautam-Arora24/zulip-mobile
that referenced
this pull request
Feb 2, 2021
Instead of spelling out the exact same code ourselves. Corresponds to the portion of facebook/react-native@619d5d60d that was missing in its backport to RN v0.62 (facebook/react-native@b4d1fcfb2); we took all of that incomplete backport in 9a144c5. After aligning our Podfile code with the corresponding code in the RN v0.63 script, in zulip#4322, this change can be done as an NFC commit after the RN v0.63 upgrade commit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are all of the Flipper-related changes that it makes sense (to me, currently) to take before the main-upgrade commit for RN v0.63 (#4245). When we get there, there will be large changes to make to our Podfile, mostly because React Native stops having us put lots of code (including Flipper-specific code) in our Podfile and instead maintains it themselves, in its
scripts/{autolink-ios.rb => react_native_pods.rb}(that rename happens in facebook/react-native@4118d7982). Cool; glad not to have to write out a bunch of stuff in our Podfile, eventually.The commits where that code is moved back to RN are these:
We've long preferred to isolate some changes that make sense to do (and are possible to do) before a main RN upgrade commit. These commits do just that, but it's important to note that I've transposed some of the changes from
scripts/{autolink-ios.rb => react_native_pods.rb}onto our Podfile because of the above. Those changes don't appear in the RN Upgrade Helper, which only looks at the template app. I still think they're quite important to do; facebook/react-native@29639e7b9, for example, has parts that are reflected in the Upgrade Helper that quite plausibly could be done before the main upgrade commit. But it's marked as depending on Flipper v0.41.0—an upgrade that gets done in that same commit, but (for iOS) inscripts/{autolink-ios.rb => react_native_pods.rb}, which we'd miss if we were only looking at the portion of that commit that touches the template app (and that makes it to the Upgrade Helper).I'll post more about individual RN commits in some comments, to follow.
[1] It looks like this commit was incompletely backported to RN v0.62, in facebook/react-native@b4d1fcfb2; we took that incomplete backport in our 9a144c5. (At the time, I think I didn't realize this was an incomplete backport or even a backport.) I say it's incomplete because, even though it adds the same RN code to
scripts/{autolink-ios.rb => react_native_pods.rb}, it doesn't invoke it; instead, it has the template app largely duplicate the code (with the Flipper version off by two patch versions; not sure why).