[RNMobile] Upgrade to RN 0.66 #36328
Merged
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
- react-native - react - react-dom - react-test-renderer - metro-react-native-babel-preset - metro-react-native-babel-transformer
These patches have now arrived from upstream changes. - facebook/metro@1e6cec8 - facebook/react-native@842bcb9
Result of `npm run native preios`.
Errors were thrown from `useSelect` and `useDispatch` returning unexpected `undefined` values. Utilize the store defintion rather than the store name string resolved the errors. There is already a movement to use this approach in general: https://git.io/JPQW2
This function is marked as deprecated.We should be replaced by calling `remove` on the subscription itself. https://git.io/JPQzO
Remove usage of deprecated `EventEmitter.removeEventListener`. This function is marked as deprecated.We should be replaced by calling `remove` on the subscription itself. https://git.io/JPQzO The patch can be removed once we upgrade to react-native-modal@^13.0.0. https://git.io/JPQgq
Set the relative RN CLI path for the Demo app to fix broken builds when using Android Studio, as opposed to the RN CLI directly. It appears the [code used to locate the CLI was modified](https://git.io/JcNzp) for v0.64.0. So, our recent React Native upgrade is likely when this issue began.
When enabled, this option converts top-level imports into inline requires as a performance optimization for large apps. However, it would appear that some import side effects break, e.g. `import './hooks'`. https://reactnative.dev/docs/ram-bundles-inline-requires#inline-requires
Remove usage of deprecated `EventEmitter.removeEventListener`. This function is marked as deprecated.We should be replaced by calling `remove` on the subscription itself. https://git.io/JPQzO The patch can be removed once we upgrade to react-navigation@^6.0.0. https://git.io/JP7OG
Installing pods resulted in the following warning. Given that the target
is an unused test workspace, it felt safe to remove this setting.
```
[!] The `GutenbergDemoTests [Debug]` target overrides the `ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES` build setting defined in `Pods/Target Support Files/Pods-GutenbergDemo-GutenbergDemoTests/Pods-GutenbergDemo-GutenbergDemoTests.debug.xcconfig'. This can lead to problems with the CocoaPods installation
- Use the `$(inherited)` flag, or
- Remove the build settings from the target.
[!] The `GutenbergDemoTests [Release]` target overrides the `ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES` build setting defined in `Pods/Target Support Files/Pods-GutenbergDemo-GutenbergDemoTests/Pods-GutenbergDemo-GutenbergDemoTests.release.xcconfig'. This can lead to problems with the CocoaPods installation
- Use the `$(inherited)` flag, or
- Remove the build settings from the target.
```
The Carthage documentation showcases this argument. Using it resolved the following error when running `npm run native preios:carthage`. https://github.com/Carthage/Carthage/tree/0.38.0#quick-start ``` Building universal frameworks with common architectures is not possible. The device and simulator slices for "Aztec" both build for: arm64 Rebuild with --use-xcframeworks to create an xcframework bundle instead. ```
The added methods are required by React Native. Otherwise, the below warning is displayed. https://stackoverflow.com/a/69650217/378228 ``` WARN `new NativeEventEmitter()` was called with a non-null argument without the required `addListener` method. WARN `new NativeEventEmitter()` was called with a non-null argument without the required `removeListeners` method. ```
`console.disableYellowBox` is deprecated. Additionally, totally disabling the logger increases the likelihood that we inadvertently overlook new warnings.
This patch changed from the deprecated `componentWillReceiveProps` to `componentDidUpdate`. We can remove this patch when we upgrade to `react-native-keyboard-aware-scroll-view@^0.9.2` https://git.io/JPbOK
Utilize the new versions of the scroll methods. This patch could be removed if we upgrade to `react-native-keyboard-aware-view@^0.9.5`. https://git.io/JPb6a
A few lines before this, `remove` is called on the subscription itself.
We should use `remove` on the subscription itself instead.
Build errors occurred for `react-native-bridge` as it attempted to target 11.0, where the editor project targets 13.0. This change lets all Pods, except RCT-Folly, inherit the target of the project. RCT-Folly requires an explicit target. - https://stackoverflow.com/a/37289688/378228 - https://git.io/JPb7Y - https://git.io/JPb73
Result of running `npm run native ios`.
dcalhoun
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
dcalhoun
force-pushed
the
chore/upgrade-to-react-native-0.66
branch
from
2b194ec
to
c9d4440
Compare
The `Clipboard` module was marked as deprecated and will be removed from React Native core. React Native documentation recommends relying upon the community package instead. ``` WARN Clipboard has been extracted from react-native core and will be removed in a future release. It can now be installed and imported from '@react-native-clipboard/clipboard' instead of 'react-native'. See https://github.com/react-native-clipboard/clipboard ```
These changes did not show up until the second run, unsure as to why.
This aligns with the mock found within the React Native source itself. Prior to this change, `AccessibilityInfo` was `undefined` within tests. https://git.io/JXKgx
These changes occurred after upgrading React Native to 0.66.2. They do not appear suspicious.
hypest
reviewed
Dec 20, 2021
hypest
reviewed
Dec 20, 2021
| @@ -350,8 +353,8 @@ export default compose( [ | |||
| getEditorBlocks, | |||
| getEditedPostAttribute, | |||
| getEditedPostContent, | |||
| } = select( 'core/editor' ); | |||
| const { getEditorMode } = select( 'core/edit-post' ); | |||
| } = select( editorStore ); | |||
There was a problem hiding this comment.
Aha, interesting that we had to use the store objects instead of the store name literals to fix useSelect and useDispatch errors, but glad you fixed it and it helps move away from the name literals even more! 👏
hypest
reviewed
Dec 21, 2021
hypest
reviewed
Dec 21, 2021
packages/react-native-bridge/android/react-native-bridge/build.gradle
Outdated
Show resolved
Hide resolved
hypest
reviewed
Dec 21, 2021
The assertions of these tests do not tell the full story of the test expectation. Namely, due to bugs within react-native-testing-library and its dependencies, the tests do not assert navigation events that should occur _after_ the keyboard dismisses. Combining async renders and mocked timers is currently quite difficult. callstack/react-native-testing-library#379 (comment) To clarify the intent of the test, the test description was expanded to describe the expectation that the keyboard is dismissed prior to the navigation event occurring. Simultaneously dismissing the keyboard and navigating has caused performance issues in our bottom sheet before. #37559
The `Clipboard` module was relocated to a non-core package. This change mirrors earlier work in this branch, but needed to be applied to additional code that merged after the initial work. I.e. a merge resolution resulted in old references showing up.
These stale references lingered from prior iterations of the React Native upgrade work.
hypest
reviewed
Dec 22, 2021
hypest
reviewed
Dec 22, 2021
hypest
reviewed
Dec 22, 2021
There was a problem hiding this comment.
I've finished a review pass over the changes here. Have to say, awesome work @dcalhoun , lots of little or far-from-little fixes all around! 👏👏
Also, sorry it took me so long to finish a pass :(
Some notes from my side:
- I didn't dive too much into the iOS side changes.
- I didn't go through the new tests in detail. Perhaps we can review those in a second pass or after merging. I figure, since those tests are new, we are not going to break something if there's an issue with any of the tests.
- One thing that jumped out on me is how outdated quite a few of the forked deps are at this stage. Feels we need some follow up work to freshen those up. Crazy idea for the forks: perhaps we can consider a monorepo with them all. Obviously not something related to the PR at hand.
Considering that the automated test jobs as well as the manual tests are green, I think this PR can be mergeable. There are some comment threads ongoing but, I don't think any are a blocker.
That said, the plan still is to not merge the RN upgrade until early next year (a couple of weeks away) when more folks will be back from AFK to help with testing and fixing any issue that might crop up. For that, I will not mark this PR as "Approved" so we don't merge it involuntarily.
The React Native Clipboard module is deprecated. This project now utilizes the recommended, third-party module `@react-native-clipboard/clipboard` instead.
@hypest wonderful! Thank you for taking the time to review this and provide helpful feedback.
Alright. @twstokes would you be willing to provide additional review as well?
Sounds good. Yes, the nature of tests should mean new tests have no impact on the app stability.
Agreed. The monorepo is an interesting idea. I think it would make updating the React Native version for all forks simpler, but it may make pulling in upstream changes for each of the repositories more difficult. 🤔 I have created #37594 to track further discussion on this.
👍🏻 Makes sense to me. |
gziolo
reviewed
Dec 22, 2021
Using ranges avoids locking third-party projects to a specific version when consuming the `@wordpress/element` package.
mchowning
reviewed
Dec 22, 2021
There was a problem hiding this comment.
Really great work @dcalhoun ! 🤩 This was a big undertaking and you did a great job on it. I really appreciated how well you commented your commits for this. Really made reviewing this not-so-small PR much easier! 🙇
I did a full code review and some very limited testing and found no real issues. I left a few minor comments, but nothing blocking. 👍
| public void removeListeners(Integer count) { | ||
| // Keep: Required for RN built in Event Emitter Calls. | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm really surprised that RN is set up so that failing to manually add these empty methods on Android results in a runtime error, but it does appear to be the way to address this. Just adding a couple of links I found useful when digging into this a bit.
facebook/react-native@114be1d
facebook/react-native-website#2791 (comment)
The code related to the comments changed in previous commits.
…rade-to-react-native-0.66
7 tasks
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.
Related PRs
Third-Party Fork PRs
Description
Upgrade React Native to 0.66. See the React Native Upgrade Helper for additional details.
Fixes #34654. Fixes wordpress-mobile/gutenberg-mobile#4219.
How has this been tested?
Performed of Writing Flow and UBE test cases.
Help & Support
Link Picker
Cover Focal Point
Cover Opacity
Screenshots
n/a
Types of changes
Chore
Checklist:
*.native.jsfiles for terms that need renaming or removal).