search

facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
116.27k stars 23.99k forks source link

feat: move notifying observers to event dispatcher #44474

Open WoLewicki opened 1 week ago

WoLewicki commented 1 week ago

Summary:

Based on the discussion starting here: https://discord.com/channels/514829729862516747/1073566663825432587/1237407161991172157, I suggest moving the call to _notifyEventDispatcherObserversOfEvent_DEPRECATED straight to RCTEventDispatcher.mm. It was previously in RCTInstance.mm which is only relevant on bridgeless mode. We want to mimic the behavior of https://github.com/facebook/react-native/blob/06eea61c19cd730cf0c14a436f042d30791c3f4a/packages/react-native/React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm#L75-L78 but without using currentBridge since it is considered bad practice: https://github.com/software-mansion/react-native-reanimated/issues/5497#issuecomment-2083400038.

Changelog:

[IOS] [CHANGED] - Move notifyObservers straight to RCTEventDispatcher.mm.

Test Plan:

See that example with stickyHeaders still works correctly on both bridgeless and bridge mode.

Videos with it on https://github.com/facebook/react-native/blob/deee037c62a7d62a349d34db427b14d3560ddf83/packages/rn-tester/js/examples/FlatList/FlatList-stickyHeaders.js example with more items for visibility:

https://github.com/facebook/react-native/assets/32481228/8b78104a-226b-466a-9f32-60ba4ec14100

https://github.com/facebook/react-native/assets/32481228/f2ca67cb-578f-45d4-954f-3249c6fa9410

https://github.com/facebook/react-native/assets/32481228/7d642923-ddda-4dd3-8f14-c9982a03bc2e

WoLewicki commented 1 week ago

Done ✅

WoLewicki commented 1 week ago

One downside I see about it is that we now add the listener on old arch too where it is not used. But it does not seem too problematic I guess?

cipolleschi commented 1 week ago

One downside I see about it is that we now add the listener on old arch too where it is not used. But it does not seem too problematic I guess?

Yeah, if nothing in the Old Architecture fires the RCTNotifyEventDispatcherObserversOfEvent_DEPRECATED event, that's not an issue!

Out of curiosity, have you tried to remove the pestering code here? Does ScrollView still works properly without it?

facebook-github-bot commented 1 week ago

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

analysis-bot commented 1 week ago
Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,495,355 -24
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,867,562 -10
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: deee037c62a7d62a349d34db427b14d3560ddf83 Branch: main

WoLewicki commented 1 week ago

Out of curiosity, have you tried to remove the pestering code here?

Forgot about this one, trying it now.

WoLewicki commented 1 week ago

Yeah, it works just fine when this line is removed 🚀 I'll remove it then!