react-native
react-native copied to clipboard
facebook
feat: move notifying observers to event dispatcher
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:
- bridgeless:
https://github.com/facebook/react-native/assets/32481228/8b78104a-226b-466a-9f32-60ba4ec14100
- bridge:
https://github.com/facebook/react-native/assets/32481228/f2ca67cb-578f-45d4-954f-3249c6fa9410
- old arch:
https://github.com/facebook/react-native/assets/32481228/7d642923-ddda-4dd3-8f14-c9982a03bc2e
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?
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?
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
| 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
Out of curiosity, have you tried to remove the pestering code here?
Forgot about this one, trying it now.
Yeah, it works just fine when this line is removed 🚀 I'll remove it then!