BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] #15894

janicduplessis · 2017-09-11T20:42:35Z

This makes the RCTEvent protocol more generic to make it easier to use the event coalescing feature for type of events other than components. This does a few other improvements that will be useful in follow up PRs.

Add RCTComponentEvent which is used instead of deprecated [sendInputEventWithName:body:] and remove that method completely (was only used at 2 places).
Make coalescingKey optional for events that return NO from canCoalesce.
Make viewTag optional for events that are not related to views.
Fast path for events that return NO from canCoalesce.
Add a missing test for event coalescing with different view tags.

Ended up making only one PR for all this since the changes are related and hard to separate.

Migration
Use a custom RCTEvent subclass with [sendEvent:] (preferred way to allow type safe events) or RCTComponentEvent.

Changelog:

[iOS] [Changed] - Remove deprecated RCTEvent method, sendInputEventWithName:body:

Test plan:

Ran RCTEventDispatcher unit tests
Tested manually in RNTester

pull-bot · 2017-09-11T20:50:56Z

@facebook-github-bot label Core Team

Attention: @shergin

Generated by 🚫 dangerJS

shergin · 2017-09-11T21:26:27Z

React/Base/RCTComponentEvent.m

+ if (self = [super init]) {
+ NSNumber *target = body[@"target"];
+ name = RCTNormalizeInputEventName(name);
+


Yea that would be better (just copied that code :))

shergin · 2017-09-11T21:28:03Z

React/Base/RCTComponentEvent.h

+#import <React/RCTEventDispatcher.h>
+
+/**
+ * Event that is dispatched to React components.


Should we discribe here that it is general purpose event? Should/can it be subclasses?

It could be subclassed but just reimplementing RCTEvent is probably better especially now that a bunch of methods are optional.

React/Base/RCTComponentEvent.h

shergin · 2017-09-11T21:38:23Z

React/Base/RCTEventDispatcher.h

+// Can be implemented for view based events that need to be coalesced
+// by it's viewTag.
+@property (nonatomic, strong, readonly) NSNumber *viewTag;
+// Coalescing related methods must only be implemented if canCoalesce


And /* **/.

shergin · 2017-09-11T21:43:50Z

React/Base/RCTEventDispatcher.h

@@ -84,12 +91,6 @@ __deprecated_msg("Subclass RCTEventEmitter instead");
 __deprecated_msg("Subclass RCTEventEmitter instead");

 /**
- * Deprecated, do not use.


We have two usage of this in internal code base, which is cool and manageable!

shergin · 2017-09-11T21:44:41Z

React/Base/RCTComponentEvent.m

+- (instancetype)initWithName:(NSString *)name body:(NSDictionary *)body
+{
+ if (self = [super init]) {
+ NSNumber *target = body[@"target"];


Why should we extract target from body? Should we pass it directly?

I think it is because target needs to be in the event body for JS so instead of putting it both in the body and passing it as an arg we just extract it from the body.

Mmm... So, it means that our API reflects details of internal implementation? Does not sound cool. No?
Do we modify body somewhere down the road before it does to serialization process? If yes, that coupling target and body cannot be counted as optimization and we have to fix our API.

We could probably pass target as a parameter and add it to the body in the init.

Yes, I think so. We have to know for sure should we do this or not. If it is natural performancewise, I would do it.

I thought the extra copy to make the NSDictionary mutable would be bad but we actually did it already here https:/facebook/react-native/pull/15894/files#diff-b9fc78351b7b976a36356ef6f08c1f79L117. So the main difference performance wise is the extra allocation of RCTEvent

shergin · 2017-09-11T21:44:59Z

React/Base/RCTComponentEvent.h

+ */
+@interface RCTComponentEvent : NSObject<RCTEvent>
+
+- (instancetype)initWithName:(NSString *)name body:(NSDictionary *)body;


Hmm, why we initialize it with untyped NSDictionary? I thought whole point of having RCTEvent objects is typesafty and explicitly.

How do you see the perpose of this class? Something simple which should replace all current untyped events? Should we have it?

I think the main use of RCTEvent is to support event coalescing. RCTComponentEvent is a generic event that is sent to a component so it could be something like a touch or scroll event and those can all have different bodies. It is also used by event blocks created by view managers (RCTDirectEventBlock, RCTBubblingEventBlock).

If someone want to make a type safe event I think the best way is to create a new class that implements RCTEvent or subclass RCTComponentEvent. An example of this is https:/facebook/react-native/blob/master/React/Views/RCTScrollView.m#L26

Yeah!
(How can it be used by RCT*EventBlock if you just added this?)
So, in my mind we have to advice to product developers use RCT*EventBlock or Custom implementation of RCTEvent. We have to discourage them to use this untyped and generic RCTComponentEvent. But, it is okay to use it as a temporary solution for migration from sendEventWithName.

RCT*EventBlock Is easy to use.

Custom RCTEvent implementation is powerful and semantically explicit.

This should be the most important point of this change. Seems that Nick Lockwood's original idea when he started this migration awhile ago.

Do you agree with this vision? Is this reasonable?

(Oh, I understood where we use it as a bearer for RCTEvent*Block, that's great! Fantastic!)

Yea this class is used internally and could also be used as an easy migration from sendEventWithName. Other events should just implement RCTEvent

shergin · 2017-09-13T17:07:59Z

@janicduplessis Thank you so much for this work! Sorry for asking so many questions, I just wont to make it right finally.

shergin · 2017-09-13T17:13:56Z

I have to also mention that is it breaking change for many external libraries, se we have to be super expressive and clear with motivation of this change and give a great advices how to migrate the old code to the reality of the world without sendEventWithName:.

janicduplessis · 2017-09-14T06:33:44Z

@shergin I think I addressed all your feedback, let me know if anything else isn't clear :)

shergin · 2017-09-16T05:14:00Z

React/Base/RCTComponentEvent.m

+- (instancetype)initWithName:(NSString *)name viewTag:(NSNumber *)viewTag body:(NSDictionary *)body
+{
+ if (self = [super init]) {
+ NSMutableDictionary *mutableBody = [NSMutableDictionary dictionaryWithDictionary:body];


Wait, that does not look right. Why do we need put tag inside body? That was needed for old poorly designed sendEventBlahBlahWithoutTagButWithBody:, right?

I'm pretty sure JS needs the tag in the body that's why we put it here. I could be wrong though I'd have to look at the RCTEventEmitter.receiveEvent code.

Yea it is definitely used https:/facebook/react/blob/7f78749ee3023608a614b2f595739bcd36acc128/src/renderers/native/ReactNativeEventEmitter.js#L112. I'd rather not touch this :)

Yeah... But... I am trying understand that code... and I failed.
Looks like we are sending tag as rootNodeID (which is not even tag!) as a first argument... Why? 🤔

Yea I'm not sure why we are sending the tag as both the first arg and in the event body as target. I just think we should keep it like this to avoid breaking react.

@shergin Let's see if someone from the react team can help here. I agree we should have a clear plan here. We also have to consider android if we make any change to the native / react bridging api.

Hi! I'm only somewhat familiar with our event code. (I don't think anyone on the core team presently is deeply familiar with it, to be honest.)

The event params like target seem to be optional given the Flow typing and the fact that we fallback to undefined values.

If the 2 values are always the same (for Android too) then it seems silly to always pass them both and I could update the ReactNativeEventEmitter to just create this wrapper object (or not, if we determine it's not actually being used for anything).

FWIW it looks like the Java implementation doesn't necessarily pass a target but it does pass other attributes in some cases (eg on content size change). The Java interface also defines the map as a @Nullable type and in at least some cases explicitly passes null.

Thanks @bvaughn, I'm pretty sure we need to keep target in the event payload otherwise it would be a breaking change for apps that rely on this value. Maybe we could avoir passing the tag as the first argument if it's always the same as body.target. Needs more investigation.

@shergin What do you think about landing this as is since it will unblock a few other improvements I wanted to make?

I'm pretty sure we need to keep target in the event payload otherwise it would be a breaking change for apps that rely on this value.

Sure! To be clear: I wasn't suggesting we remove event.target on the JS side, just that we could remove the duplicate parameter and (on the RN JavaScript side) just always set target equal to the tag number.

But that's obviously kind of a polish detail and could be done, or not, at any time.

janicduplessis · 2017-10-12T21:36:01Z

@shergin Could you have a look at this soon? :)

shergin · 2018-01-23T21:08:40Z

@janicduplessis Definitely.

lnikkila · 2018-05-14T15:06:39Z

Is this PR still being considered?

I’m interested since it seems this makes it possible to use the native animation driver with onLayout and Animated.event. Currently the native driver never sees the event since it’s being sent by an event block which uses sendInputEventWithName and doesn’t notify observers.

EDIT: Just noticed #15611 which addresses exactly this issue... 😄 I guess I’ll leave this comment here to express my interest in keeping this PR alive, let me know if I can help somehow.

janicduplessis · 2018-05-14T22:07:00Z

@lnikkila Yes, the initial goal was to fix the case you mentioned, but it required additional changes to be able to do it in a clean way.

janicduplessis · 2018-05-14T22:09:53Z

ping @shergin if you can find some time to revisit this it would be nice

hramos · 2019-02-27T21:34:07Z

I'm looking into whether I can patch react-native-fbsdk as part of this diff. Once that's done, I'll land this.

pull-bot · 2019-03-01T19:22:08Z

	Messages
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖	📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`.

Generated by 🚫 dangerJS against 7bf91d5

zhongwuzw · 2019-03-14T04:09:11Z

React/Base/RCTComponentEvent.h

+ */
+@interface RCTComponentEvent : NSObject<RCTEvent>
+
+- (instancetype)initWithName:(NSString *)name viewTag:(NSNumber *)viewTag body:(NSDictionary *)body;


We need to add some nonnull attributes to prevent crash?

This file could use NS_ASSUME_NONNULL_BEGIN, feel free to open a PR :)

React/Base/RCTEventDispatcher.m

react-native-bot · 2019-03-27T18:25:05Z

This pull request was successfully merged by @janicduplessis in 41343f6.

^{When will my fix make it into a release? | Upcoming Releases}

shergin · 2019-04-01T17:11:53Z

🎉

sahrens · 2019-04-23T23:43:27Z

Heads up I think this (still working on confirming if this PR is the cause) might be causing some crashes with missing touches and some bugs with Animations where some scroll events seem to be getting dropped, causing screens to get out of sync. Easiest way to repro this something like this:

<Animated.ScrollView
          onScroll={Animated.event(
            [{nativeEvent: {contentOffset: {y: scrollOffset}}}],
            {useNativeDriver: true},
          )}>
  <Animated.View
    style={{
      height: 100,
      backgroundColor: 'cyan',
      transform: [{translateY: scrollOffset}]}}>
      <Text>FOO</Text>
  </Animated.View>
  <View style={{height: 1000, backgroundColor: 'red'}} />
</Animated.ScrollView>

If you scroll aggressively you'll see some jumping around of the cyan view/text when it should stay fixed in place.

shergin · 2019-04-30T19:04:31Z

Hey Janic,

I really do love this PR but we have serious suspicion that this PR can cause a significant crash spike that we are observing (T42979947).

We are seriously considering reverting it but I hope we can figure out the issue without that (with your help). Seems the issue is related to touch event dispatching process and seems that some events are getting lost in the process. Can it be related to changes in coalescing mechanism?

Details:

 "assert_message": "<level:warning> QuickReport generated: Unhandled JS Exception: <force_category:console.error: \"Cannot record touch move without a touch start.\nTouch Move: %s\n\", \"Touch Bank: %s\", \"{\"identifier\":2,\"pageX\":206.5,\"pageY\":361,\"timestamp\":79597115.8785}\", \"[null,{\"touchActive\":true,\"startPageX\":168.5,\"startPageY\":516.5,\"startTimeStamp\":79588038.00445834,\"currentPageX\":170.5,\"currentPageY\":510.5,\"currentTimeStamp\":79590372.55025001,\"previousPageX\":168.5,\"previousPageY\":516.5,\"previousTimeStamp\":79588038.00445834}]\", js engine: hermes:force_category>, stack:\npe@1:411668\nrecordTouchTrack@1:455342\nextractEvents@1:455483\nanonymous@1:412884\nze@1:459961\nAe@1:412461\nMe@1:412794\nreceiveTouches@1:457566\nvalue@1:289603\nanonymous@1:288156\nvalue@1:289192\nvalue@1:288114\n",
 "symbolicated_js_trace": "<level:warning> QuickReport generated: Unhandled JS Exception: <force_category:console.error: \"Cannot record touch move without a touch start. Touch Move: %s \", \"Touch Bank: %s\", \"{\"identifier\":2,\"pageX\":206.5,\"pageY\":361,\"timestamp\":79597115.8785}\", \"[null,{\"touchActive\":true,\"startPageX\":168.5,\"startPageY\":516.5,\"startTimeStamp\":79588038.00445834,\"currentPageX\":170.5,\"currentPageY\":510.5,\"currentTimeStamp\":79590372.55025001,\"previousPageX\":168.5,\"previousPageY\":516.5,\"previousTimeStamp\":79588038.00445834}]\", js engine: hermes:force_category>, stack: /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:533:recordTouchMove /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:575:ResponderTouchHistoryStore.recordTouchTrack /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:710:ResponderEventPlugin.extractEvents /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:1049:batchedUpdates$argument_0 /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:7078:_batchedUpdatesImpl /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:1024:batchedUpdates /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:1044:_receiveRootNodeIDEvent /js/react-native-github/Libraries/Renderer/fb/ReactNativeRenderer-prod.js:1109:RCTEventEmitter.register$argument_0.receiveTouches /js/react-native-github/Libraries/BatchedBridge/MessageQueue.js:395:MessageQueue#__callFunction /js/react-native-github/Libraries/BatchedBridge/MessageQueue.js:106:__guard$argument_0 /js/react-native-github/Libraries/BatchedBridge/MessageQueue.js:343:MessageQueue#__guard /js/react-native-github/Libraries/BatchedBridge/MessageQueue.js:105:MessageQueue#callFunctionReturnFlushedQueue",
 "exception_message": "<level:warning> QuickReport generated: Unhandled JS Exception: <force_category:console.error: \"Cannot record touch move without a touch start.\nTouch Move: %s\n\", \"Touch Bank: %s\", \"{\"identifier\":2,\"pageX\":206.5,\"pageY\":361,\"timestamp\":79597115.8785}\", \"[null,{\"touchActive\":true,\"startPageX\":168.5,\"startPageY\":516.5,\"startTimeStamp\":79588038.00445834,\"currentPageX\":170.5,\"currentPageY\":510.5,\"currentTimeStamp\":79590372.55025001,\"previousPageX\":168.5,\"previousPageY\":516.5,\"previousTimeStamp\":79588038.00445834}]\", js engine: hermes:force_category>"

cc @hramos

janicduplessis · 2019-05-01T01:08:23Z

@shergin I reviewed the code and it seems like when the event is not coalescable we are no longer adding it the the event queue and just dispatching it directly. I think this can cause events to arrive in the wrong order when combining coalescable and non-coalescable events.

Are you able to reproduce the crash consistently? I can put up a PR with a potential fix and you can see if it works.

…nts (#24693) Summary: Fixes an issue introduced in #15894 that can cause events to be dispatched out of order. Also reverted `viewTag` moved to optional property as it didn't actually work and makes code more complex. [iOS] [Fixed] - Fix event ordering when combining coalescable and non-coalescable events Pull Request resolved: #24693 Differential Revision: D15279513 Pulled By: shergin fbshipit-source-id: 3c64aba6d644ea9564572e6de8330b59b51cf4a9

janicduplessis added 2 commits September 11, 2017 15:44

Event improvements

0ecb4ee

Remove sendInputEvent

df92f4a

janicduplessis requested a review from shergin as a code owner September 11, 2017 20:42

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 11, 2017

janicduplessis changed the title ~~Event improvements~~ RCTEvent improvements Sep 11, 2017

janicduplessis changed the title ~~RCTEvent improvements~~ RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] Sep 11, 2017

shergin reviewed Sep 11, 2017

View reviewed changes

janicduplessis changed the title ~~RCTEvent improvements, remove deprecated [sendInputEventWithName:body:]~~ BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] Sep 14, 2017

janicduplessis added 2 commits September 14, 2017 02:20

Pass viewTag as an argument instead of in the event body

c58ea62

Update comments

a72e0f4

shergin suggested changes Sep 16, 2017

View reviewed changes

shergin mentioned this pull request Sep 30, 2017

Native Animated - Support events using RCT{Direct|Bubbling}EventBlock on iOS #15611

Closed

Merge branch 'master' into event-improvements

424e96a

stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 11, 2018

stale bot closed this Jan 18, 2018

shergin reopened this Jan 23, 2018

stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 23, 2018

hramos removed the GH Review: review-needed label Mar 8, 2018

react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018

hramos mentioned this pull request Feb 14, 2019

Native Animated - Coalesce onValueChanged events on iOS #15682

Closed

hramos mentioned this pull request Feb 27, 2019

Use RCTComponentEvent facebookarchive/react-native-fbsdk#487

Merged

Merge branch 'master' into event-improvements

7bf91d5

hramos added Needs: Imported Diff Waiting on Meta Partner Contributor A React Native contributor. and removed Needs: Imported Diff Review Core Team labels Mar 2, 2019

hramos mentioned this pull request Mar 13, 2019

[iOS] Fixed crash if event doesn't have body parameter #23711

Closed

react-native-bot added Platform: iOS iOS applications. PR: Includes Changelog labels Mar 14, 2019

zhongwuzw reviewed Mar 14, 2019

View reviewed changes

hramos removed the PR: Includes Changelog label Mar 14, 2019

facebook-github-bot closed this in 41343f6 Mar 27, 2019

react-native-bot added Merged This PR has been merged. and removed Needs: Imported Diff Waiting on Meta labels Mar 27, 2019

janicduplessis mentioned this pull request Apr 19, 2019

onLayout is not triggered when Animated.event applied on iOS #24219

Closed

hramos removed Partner labels Apr 24, 2019

janicduplessis mentioned this pull request May 2, 2019

Fix event ordering when combining coalescable and non-coalescable events #24693

Closed

BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] #15894

BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] #15894

Conversation

janicduplessis commented Sep 11, 2017 • edited by hramos Loading

Changelog:

Test plan:

pull-bot commented Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shergin commented Sep 13, 2017

shergin commented Sep 13, 2017

janicduplessis commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

janicduplessis commented Oct 12, 2017

shergin commented Jan 23, 2018

lnikkila commented May 14, 2018 • edited Loading

janicduplessis commented May 14, 2018

janicduplessis commented May 14, 2018

hramos commented Feb 27, 2019

pull-bot commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

react-native-bot commented Mar 27, 2019

shergin commented Apr 1, 2019

sahrens commented Apr 23, 2019

shergin commented Apr 30, 2019

janicduplessis commented May 1, 2019

janicduplessis commented Sep 11, 2017 •

edited by hramos

Loading

pull-bot commented Sep 11, 2017 •

edited

Loading

janicduplessis Sep 11, 2017 •

edited

Loading

bvaughn Sep 22, 2017 •

edited

Loading

lnikkila commented May 14, 2018 •

edited

Loading