chore(Android): migrate from deprecated `RCTEventEmitter` #1867

kkafar · 2023-08-09T18:22:35Z

Description

RCTEventEmitter was deprecated over 3 years ago in this commit

I don't know when it will be finally deleted, but:

There is no reason to wait for this moment with migration
According to the commit message there can be some minor performance improvements on Fabric
People complain on wall of warnings during build process

I inspected ReactNative's source code and it seems that parent class impl of Event.dispatch method we used up to this point calls Event.getEventData internally and if anything is returned it dispatches the event using appropriate event emitter (see here).

Note: This PR breaks support for versions of react-native lower than 0.65.x.

Changes

In all event classes:

Removed Event.dispatch override
Overrided Event.getEventData
Added surfaceId to event ctor

In all event construction places:

Resolved appropriate (🤞🏻) surfaceId
Updated event construction

Test code and steps to reproduce

My empirical impression: it works as earlier.
My methodical attempt to test it:

Add console logs to event handlers in RouteView in src/native-stack/views/NativeStackView.tsx use e.g. Test1072 or different ones
Play around with text example
Notice that events are received in JS as expected.

Checklist

Included code example that can be used to test this change
Ensured that CI passes

kkafar

Just few remarks for reviewer.

android/src/main/java/com/swmansion/rnscreens/SearchBarView.kt

kkafar · 2023-08-10T11:42:19Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.kt

@@ -192,11 +192,12 @@ open class ScreenFragment : Fragment {
        if (fragment is ScreenStackFragment && fragment.canDispatchEvent(event)) {
            fragment.screen.let {
                fragment.setLastEventDispatched(event)
+                val surfaceId = UIManagerHelper.getSurfaceId(it)


I believe on Paper surfaceId gets resolved always to -1 -- it is Paper compatible.

Did you test it on the lowest version of RN we support?

UIManagerHelper.getSurfaceId exists since 0.65 -> that leaves 0.64 behind -> this PR might wait for a while before it is merged. But this situation nudges me towards thoughts that maybe we should reopen the discussion on what should be the minimal supported RN version, because it is 8 versions back right now.

WoLewicki

Just a couple of questions 🚀

WoLewicki · 2023-08-11T09:47:48Z

android/src/main/java/com/swmansion/rnscreens/ScreenFragment.kt

@@ -192,11 +192,12 @@ open class ScreenFragment : Fragment {
        if (fragment is ScreenStackFragment && fragment.canDispatchEvent(event)) {
            fragment.screen.let {
                fragment.setLastEventDispatched(event)
+                val surfaceId = UIManagerHelper.getSurfaceId(it)


Did you test it on the lowest version of RN we support?

android/src/main/java/com/swmansion/rnscreens/SearchBarView.kt

WoLewicki · 2023-08-11T10:06:22Z

android/src/main/java/com/swmansion/rnscreens/events/ScreenDismissedEvent.kt

-        val args = Arguments.createMap()
-        // on Android we always dismiss one screen at a time
-        args.putInt("dismissCount", 1)
-        rctEventEmitter.receiveEvent(viewTag, eventName, args)


Are you sure we can just remove this code? Is it done like this in other libs/core ?

Just take a look into default implementation of this method.

Also I've just found an example

tboba

That's a yes from me! But as @WoLewicki said, before merging it let's discuss when we should support RN <0.65 (<0.66?) 👍

kkafar · 2023-08-31T07:36:02Z

As we've reached agreement on dropping RN versions and I've just released 3.25.0 with already merged changes I'm merging this one.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [react-native-screens](https://togithub.com/software-mansion/react-native-screens) | [`^3.25.0` -> `^3.26.0`](https://renovatebot.com/diffs/npm/react-native-screens/3.25.0/3.26.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-native-screens/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-native-screens/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-native-screens/3.25.0/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-native-screens/3.25.0/3.26.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>software-mansion/react-native-screens (react-native-screens)</summary> ### [`v3.26.0`](https://togithub.com/software-mansion/react-native-screens/releases/tag/3.26.0) [Compare Source](https://togithub.com/software-mansion/react-native-screens/compare/3.25.0...3.26.0) Minor release adding new useAnimatedHeaderHeight and useReanimatedHeaderHeight hooks, providing fixes for search bar and introducing internal changes. *Please note that new hooks introduced in this release are fully functional on Paper, on Fabric there are few edge cases we are still working on*. #### What's Changed #### 🐛 Bug fixes - Change implementation of `headerConfig` prop on Android by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1883 - Change elements visibility on search bar open by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1903 - Fix positioning of large header and search bar by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1895 - Change implementation of calculating status bar, refactor methods used on header height change by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1917 - Fix calculating header height when changing status/action bar visibility by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1922 - Allow Reanimated Screen to check large header by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1915 - Fix issue when emptying nav stack on Windows by [@chrisglein](https://togithub.com/chrisglein) in [software-mansion/react-native-screens#1890 - Update podspec to use install_modules_dependencies by [@cipolleschi](https://togithub.com/cipolleschi) in [software-mansion/react-native-screens#1920 - Remove MaxPerm args from JVM invocation by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1888 #### 👍 Improvements - Calculate values of useHeaderHeight natively by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1802 - Allow for different fragment types inside ScreenContainer by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1887 - Add focused states on page transitions by [@tboba](https://togithub.com/tboba) in [software-mansion/react-native-screens#1894 #### 🔢 Miscellaneous - **Create FUNDING.yml by [@aleqsio](https://togithub.com/aleqsio) in [software-mansion/react-native-screens#1886 - Migrate from deprecated `RCTEventEmitter` by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1867 - Use `require` syntax for resolution of all native components by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1909 - Run Android e2e with JDK 17 by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1892 - Put timelimit on execution of each workflow by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1893 - Trigger e2e tests on JS-only changes by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1910 - Update deprecated expo install instructions to `npx expo install` by [@GabrieldosSantosOliveira](https://togithub.com/GabrieldosSantosOliveira) in [software-mansion/react-native-screens#1899 - Bump activesupport from 6.1.7.3 to 7.0.7.2 in /TestsExample by [@dependabot](https://togithub.com/dependabot) in [software-mansion/react-native-screens#1877 - Update deps & RN in example apps after release by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1878 - Migrate `Example` app & e2e tests to RN 0.72.4 by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1880 - Bump library deps to recent versions (including RN) by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1881 - Bump library native Android deps & config by [@kkafar](https://togithub.com/kkafar) in [software-mansion/react-native-screens#1891 #### New Contributors - [@chrisglein](https://togithub.com/chrisglein) made their first contribution in [software-mansion/react-native-screens#1890 - [@GabrieldosSantosOliveira](https://togithub.com/GabrieldosSantosOliveira) made their first contribution in [software-mansion/react-native-screens#1899 - [@cipolleschi](https://togithub.com/cipolleschi) made their first contribution in [software-mansion/react-native-screens#1920 **Full Changelog**: software-mansion/react-native-screens@3.25.0...3.26.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone America/Los_Angeles. 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/valora-inc/wallet).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: valora-bot <valorabot@valoraapp.com> Co-authored-by: Silas Boyd-Wickizer <silasbw@gmail.com> Co-authored-by: Silas Boyd-Wickizer <silas@valora.xyz>

@kkafar

## Description It turns out that our code uses some deprecated functions. Also there were few more warnings, such as shadowed name or unused variables. Some of included changes require digging into `react-native` source code to find alternative approach. Fixes #2460 ## `dispatch` method Based on @kkafar research in [screens](software-mansion/react-native-screens#1867), I've replaced `dispatch` method with `getEventData`. - [Here](https://github.com/facebook/react-native/blob/e2ef6b98a061508bdd70a4c2747fe061c49b5c39/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/events/Event.java#L154-L168) you can find how `dispatch` looks like in the source code. - [Here](facebook/react-native@72d0ddc#diff-afc6f25f459dcf1e24a012e16862c869aa04b0de5795171efe14b7a88be08e64) you can see example change in react-native ## Test plan Tested that our example apps (paper and fabric) work as expected.

kkafar added 5 commits August 9, 2023 20:21

Migrate HeaderAttachedEvent & HeaderDetachedEvent

cc476f8

Migrate HeaderBackButtonClickedEvent

043c862

Migrate ScreenAppearEvent

adb7ca3

Migrate ScreenDisappearEvent

11c3f7c

Migrate ScreenDismissedEvent

ca261ac

kkafar force-pushed the @kkafar/migrate-from-deprecated-rct-event-emitter branch from 1ddc6f6 to ca261ac Compare August 9, 2023 18:45

Migrate rest of the events

ea09487

kkafar requested review from WoLewicki and tboba August 10, 2023 11:37

kkafar commented Aug 10, 2023

View reviewed changes

WoLewicki reviewed Aug 11, 2023

View reviewed changes

tboba approved these changes Aug 16, 2023

View reviewed changes

kkafar merged commit 30ee316 into main Aug 31, 2023
4 checks passed

kkafar deleted the @kkafar/migrate-from-deprecated-rct-event-emitter branch August 31, 2023 07:36

ali-sao mentioned this pull request Jan 7, 2024

Swipe is lost when navigating from a route to another. react-native-tvos/react-native-tvos#629

Closed

m-bert mentioned this pull request Mar 7, 2024

Address compilation warnings software-mansion/react-native-gesture-handler#2597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(Android): migrate from deprecated `RCTEventEmitter` #1867

chore(Android): migrate from deprecated `RCTEventEmitter` #1867

kkafar commented Aug 9, 2023 •

edited

kkafar left a comment

kkafar Aug 10, 2023

WoLewicki Aug 11, 2023

kkafar Aug 14, 2023

WoLewicki left a comment

WoLewicki Aug 11, 2023

WoLewicki Aug 11, 2023

kkafar Aug 14, 2023

kkafar Aug 14, 2023

tboba left a comment

kkafar commented Aug 31, 2023

chore(Android): migrate from deprecated RCTEventEmitter #1867

chore(Android): migrate from deprecated RCTEventEmitter #1867

Conversation

kkafar commented Aug 9, 2023 • edited

Description

Changes

Test code and steps to reproduce

Checklist

kkafar left a comment

Choose a reason for hiding this comment

kkafar Aug 10, 2023

Choose a reason for hiding this comment

WoLewicki Aug 11, 2023

Choose a reason for hiding this comment

kkafar Aug 14, 2023

Choose a reason for hiding this comment

WoLewicki left a comment

Choose a reason for hiding this comment

WoLewicki Aug 11, 2023

Choose a reason for hiding this comment

WoLewicki Aug 11, 2023

Choose a reason for hiding this comment

kkafar Aug 14, 2023

Choose a reason for hiding this comment

kkafar Aug 14, 2023

Choose a reason for hiding this comment

tboba left a comment

Choose a reason for hiding this comment

kkafar commented Aug 31, 2023

chore(Android): migrate from deprecated `RCTEventEmitter` #1867

chore(Android): migrate from deprecated `RCTEventEmitter` #1867

kkafar commented Aug 9, 2023 •

edited