[RNMobile] Upgrade React Native 0.71.11 - iOS changes
#51386
Conversation
Without this patch, Reanimated tries to use Hermes version of React Native and produced a build failure. Seems there's an issue in the `podspec` file, as the JSC module is not being added. Reference: software-mansion/react-native-reanimated#4254
fluiddot
added
the
Mobile App - i.e. Android or iOS
There was a problem hiding this comment.
I encountered this issue (software-mansion/react-native-reanimated#4254) when building the demo app using Reanimated 2.17.0. I noticed in the error logs that it was referencing React-Hermes, which make me think that probably the library wasn't picking the right version (i.e. JSC). For now, we could use this patch, although it would be great to update the library once a fix lands in a new patch version.
Error:
ld: warning: directory not found for option '-F/Users/<USER>/Library/Developer/Xcode/DerivedData/GutenbergDemo-eijjnimdkxoyazgtpfhkeoxkitnu/Build/Products/Debug-iphonesimulator/React-hermes'
Undefined symbols for architecture arm64:
"facebook::jsc::makeJSCRuntime()", referenced from:
reanimated::createReanimatedModule(RCTBridge*, std::__1::shared_ptr<facebook::react::CallInvoker>) in NativeProxy.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Directory not found for option '-F/Users/<USER>/Library/Developer/Xcode/DerivedData/GutenbergDemo-eijjnimdkxoyazgtpfhkeoxkitnu/Build/Products/Debug-iphonesimulator/React-hermes'
Undefined symbol: facebook::jsc::makeJSCRuntime()
There was a problem hiding this comment.
May not be necessary, but possibly worth including an inline comment stating when we could remove this patch. This PR comment may become difficult to find.
| # dependencies: { | ||
| # ...(process.env.NO_FLIPPER ? { 'react-native-flipper': { platforms: { ios: null } } } : {}), | ||
| # ``` | ||
| flipper_config = ENV['NO_FLIPPER'] == "1" ? FlipperConfiguration.disabled : FlipperConfiguration.enabled |
There was a problem hiding this comment.
@fluiddot, a question out of curiosity more than any strong opinion: I noticed that suggested changes related to flipper were left out from the 0.66.2 → 0.69.4 PR, which is why I didn't include flipper-related changes in my commits. What is your thought process for including it? I'm guessing that it's perhaps due to it not causing any issues and being nice to have setup, just in case?
There was a problem hiding this comment.
Yeah, I noticed that they weren't included in the last RN upgrade. I understand it wasn't added because we don't use Flipper in the demo app, but not completely sure. cc @geriux @derekblank in case they have any insights.
From my side, I've included it in the spirit to make the Podfile look as close as the one from the upgrade helper. Seems it won't affect the build configuration, so we could leave it in case we want to enable Flipper in the future.
There was a problem hiding this comment.
For sake of discussion, my approach generally aligns with @fluiddot.
As much as possible, I make our code resemble the upgrade helper or React Native template. I do this (1) to make future upgrades easier and (2) I lack confidence in managing these native files, so the less divergence from core the more confidence I have. I think it is generally a good practice to avoid "dead" code, but in this context I make exceptions for the reasons I outlined.
I also recognize that our code will need to diverge from the core template as our product is embedded in a host app, rather than being a standalone React Native app. So, we will ultimately always have custom native code to manage.
In regards to Flipper, my perception is that we are currently blocked from using it on iOS due to our (required) usage of use_frameworks!. Hopefully, we can one day enable deeper Flipper integration for the iOS Demo app, as well as setting it up for WordPress-Android and WordPress-iOS.
There was a problem hiding this comment.
I understand it wasn't added because we don't use Flipper in the demo app.
Yes, I recall this being the reason Flipper code was excluded from the 0.69.4 upgrade. However, I agree that it will be helpful for future upgrades to make all template updates from the upgrade helper. 👍
SiobhyB
changed the title
0.71.8 - iOS changes0.71.11 - iOS changes
| @@ -0,0 +1 @@ | |||
| export NODE_BINARY=$(command -v node) | |||
There was a problem hiding this comment.
This file is being consistently added whenever I run pod install, though it does not appear in the RN upgrade helper. Interested to hear thoughts on including it.
There was a problem hiding this comment.
I believe it should be committed, and individuals can create a .xcode.env.local to modify the configuration if necessary for their individual local environment. Its exclusion was likely an oversight from prior upgrade work.
We should add .xcode.env.local to this project's .gitignore configuration.
There was a problem hiding this comment.
Thanks for the confidence check, David! I just found that .xcode.env.local is already in packages/react-native-editor/.gitignore, so it seems we're all set on that front:
https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-editor/.gitignore#L64
…rade-react-native-0.71.8-ios # Conflicts: # packages/react-native-editor/ios/Podfile.lock
…rade-react-native-0.71.8-ios
…rade-react-native-0.71.8-ios
fluiddot
merged commit 305db7f
into
rnmobile/upgrade/react-native-0.71.8
* Upgrade `react-native` dependency * Upgrade `@babel/runtime` dependency * Upgrade `metro-react-native-babel` preset and transformer dependencies * Upgrade `cocoapods` gem * Re-apply `react-devtools-core` patch to new version * Update jest snapshots with new a11y values * Mock `Linking.addEventListener` function `Linking.removeEventListener` has been removed in RN `0.71`. The library is mocked by default but doesn't return the `remove` function when calling `addEventListener`. * Update tests that fail due to use of debounce and link suggestions * Fix `MediaUpload` component test * Update `@react-navigation/native` package to version `6.0.14` * Update `react-native-reanimated` to version `2.17.0` * Update `react-native-gesture-handler` to version `2.10.2` * Fix `act` warnings produced during block insertion * Fix `act` warnings in Columns block tests * Fix `act` warnings in List block tests * Upgrade `react-native` dependency to version `0.71.11` It also upgrades `metro-react-native-babel` dependencies following the upgrade helper. * Mock return value of Linking `addEventListener` We only need to mock the return the value, hence we don't need to mock the entire library. * Remove `waitForModalVisible` usage in Paragraph block tests * Remove `waitFor` usage in Link settings tests * test: Fix act warning by awaiting LinkPicker loading indicator removal The loading indicator is displayed and subsequently removed once the suggestion fetches resolve. Explicitly awaiting this element's removal fixes the `act` warnings. * build: Update react-native-safe-area-context to 4.6.3 * build: Upgrade react-native-screens to 3.22.0 * build: Upgrade react-native-svg to 13.9.0 Based on the release notes breaking changes, we should look out for odd sizing or display of icons, particularly on Android. * build: Upgrade @react-native-masked-view/masked-view to 0.2.9 * build: Upgrade @react-native-clipboard/clipboard to 1.11.2 * build: Upgrade react-native-modal to 13.0.1 * test: Update link modal snapshot This change is a result of applying new props from the RN upgrade to a newly introduced snapshot in trunk: 71d2dc5 * Update `@react-navigation/stack` to version `6.3.5` * Upgrade `react-native-linear-gradient` to version `2.7.3` This commit also updates the `react-native-hsv-color-picker` library to point to the same version of `react-native-linear-gradient`. * Use `react-native-safe-area-context` mock provided by the library * Update link modal snapshot * Update `package-lock.json` file The integrity checksum of `react-native-hsv-color-picker` changed because the package has been modified (ref: wordpress-mobile/react-native-hsv-color-picker#10 (comment)) * Disable `react-native-screens` in navigators `react-native-screens` is meant to be used at root level to save memory when having inactive screens. This is not the case of the editor, as the stack navigators are used within the Bottom sheet component. As a side note, enabling `react-native-screens` here leads to the editor crashing. * Fix render order of animated view to highlight selected segment Rendering the animated view before the segments will ensure that is rendered behind them. * Update source of `react-native-hsv-color-picker` to use tag version * Revert "Update link modal snapshot" This reverts commit 7988b0e. This is needed after disabling `react-native-screens` in navigators (ref: e5838f4). * [RNMobile] Upgrade React Native `0.71.11` - iOS changes (#51386) * refactor: Extract bundle version number to var * refactor: Delete /.ruby-version, no longer needed * refactor: Update /podfile to align w/RN updates * refactor: Remove path names as part of RN upgrade * Update `Podfile` with changes from RN upgrade helper * Fix React Native path for `react_native_post_install` script * Update Pods * Add patch to fix Reanimated podspec Without this patch, Reanimated tries to use Hermes version of React Native and produced a build failure. Seems there's an issue in the `podspec` file, as the JSC module is not being added. Reference: software-mansion/react-native-reanimated#4254 * Update pods to reflect 0.71.11 target * Apply changes to pods following `pod install` * Update `Podfile.lock` file --------- Co-authored-by: Siobhan <siobhan@automattic.com> * [RNMobile] Upgrade React Native `0.71.11` - Android changes (#51289) * Upgrade Gradle to version 7.5.1 * Upgrade Gradle plugin * Remove no longer needed files in new version * Update Flipper initialization * Update demo project main application * Remove gradle download task plugin * Bump ndk version * Remove no longer need logic related to `newArchEnabled` * Apply plugin React Native Gradle plugin * Use React Native and Hermes modules from Maven We no longer need to publish these binaries because React Native team is publishing React Native binaries to Maven. * Remove exclude group from Flipper * Update comments in `build.gradle` to align with new RN version * Remove deprecated Gradle property * Add `mavenLocal` repository to allow testing local binaries * Bump Reanimated and Gesture handler libraries * Revert "Upgrade Gradle plugin" This reverts commit 82764a2. * build: Resolve react-native-gradle-plugin incompatability Due to host app requirements, we must use AGP 7.2.1. The included patch disables logic requiring AGP 7.3. The logic appears to not be required for our use cases in the Demo editor or host apps. We should remove this patch once we upgrade past AGP 7.3. * Reduce priority of `mavenLocal` repository in Android build configurations Maven local is used to provide dependencies located locally, which is mainly used for testing and debugging. Hence, published dependencies should be prioritized over local ones. * Disable `react-native-screens` in navigators `react-native-screens` is meant to be used at root level to save memory when having inactive screens. This is not the case of the editor, as the stack navigators are used within the Bottom sheet component. As a side note, enabling `react-native-screens` here leads to the editor crashing. * Fix render order of animated view to highlight selected segment Rendering the animated view before the segments will ensure that is rendered behind them. * Bump Linear gradient Android library This library is now published via the `react-native-libraries-publisher` repository. --------- Co-authored-by: David Calhoun <github@davidcalhoun.me> * Avoid exception in E2E tests when typing an empty string on Android * Update a11y id queries for Android E2E tests Starting in React Native 0.71, the accessibility hint is no longer appended to the accessibility content description. Reference: facebook/react-native@0b70b38 * Update button inline appender query for Android E2E tests * Unify press keycode function for E2E tests * Update comments in functions related to pressing a keycode * Update block drop position using Reanimated's shared value Seems there's some kind of incompatibility on calling a JS function from a worklet invoked from a gesture handler. For this reason, the logic to set the dropping insertion point has been updated. It now uses a Reanimated's shared value to keep the dragging over position and `useDerivedValue` hook to listen for changes. * Remove unneeded `hidden` param in Paragraph block test case Co-authored-by: David Calhoun <github@davidcalhoun.me> * Revert removing `.ruby-version` file * Add inline comment in Reanimated patch * Use `waitForElementToBeRemoved` in Paragraph block test cases This way we can avoid waiting for any microtasks of link suggestions. * Remove `act` statements from Link Settings test cases * fix: Cover focal point drag handle visibility The lack of an explicit width or height resulted in a invisible drag handle. The logic passing the dimensions to the SVG expected a single style object. The reality is that it (1) referenced only the Sass styles and (2) the combined reference was actually an array of style objects. Updating the reference and flattening it ensures the appropriate width and height are passed to the SVG. It appears the absence of explicit dimensions was not an issue in earlier versions of React Native, but it makes sense why it might be required. * [RNMobile] Use Reanimated in bottom sheet height animation (#52563) * Expose max height properties in `BottomSheetProvider` * Animate bottom sheet's height with Reanimated Pass `currentHeight` in bottom sheet navigation context * Use pixel value when setting fullscreen height We need to pass pixel values in order to animate the height with Reanimated. * Rename `heightRef` to `maxHeight` * Re-enable `exhaustive-deps` lint rule in `BottomSheetNavigationContainer` * Avoid setting height using debounce * Add test ID to navigation container component * Mock Reanimated's `now` function * Update test cases related to bottom sheet height animation * Update test snapshots * Update `react-native-editor` changelog * Drop unsupported `--no-jetifier` from Android cmd The `--no-jetifier` option no longer appears to be supported and results in an error when attempting to build the Android demo app. Ref: wordpress-mobile/gutenberg-mobile#5881 (comment) * Revert accidental change to .ruby-version * Restore correct dependencies to package-lock.json * Update `react-native-editor` changelog * Update `package-lock.json` file to revert previous conflict resolutions In 4482b9d we had a conflict in `package-lock.json` that was solved using the changes from this branch. However, seems that something went wrong and that although the editor has no issues, some e2e tests are failing due to this. This has been solved by using the latest version of `package-lock.json` file from `trunk` and updating it with the package updates required in the React Native upgrade. * Re-apply `react-devtools-core` patch to new version * Update `Podfile.lock` file --------- Co-authored-by: David Calhoun <github@davidcalhoun.me> Co-authored-by: Siobhan <siobhan@automattic.com>
Related PRs:
0.71.11- iOS changes wordpress-mobile/gutenberg-mobile#58810.71.11wordpress-mobile/WordPress-iOS#20956What?
Applies the needed changes to iOS native files to support the upgrade of React Native to version
0.71.11.Why?
This PR is needed as part of the effort of upgrading React Native to version
0.71.11.How?
Most of the changes have been applied following the React Native upgrade helper. Here is a list of the different commits applied and some notes:
react-native-editor/.ruby-versionfilepodfileconfigurationpodfileconfiguration to include Flipper configreact_native_post_installscript.xcode.env.localconfigPodfile.lockfileREACT_NATIVE_PATH0.71.11targetpodspecfile, as the JSC module is not being added. Reference: build framework Undefined symbols for architecture arm64 ,"facebook::jsc::makeJSCRuntime()", referenced from: software-mansion/react-native-reanimated#4254NOTE: I tried to update
AppDelegate, as described in the upgrade helper, but wasn't working as expected due to the custom configuration we have with Gutenberg. I noticed it's not mandatory (reference), so since it's working as-is, I've skipped this change.Testing Instructions
Follow testing instructions from #51303.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A