BREAKING - RCTEvent improvements, remove deprecated [sendInputEventWithName:body:] #15894
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| */ | ||
|
|
||
| #import <React/RCTEventDispatcher.h> | ||
|
|
||
| /** | ||
| * Event that is dispatched to React components. | ||
| */ | ||
| @interface RCTComponentEvent : NSObject<RCTEvent> | ||
janicduplessis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| - (instancetype)initWithName:(NSString *)name body:(NSDictionary *)body; | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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://github.com/facebook/react-native/blob/master/React/Views/RCTScrollView.m#L26 There was a problem hiding this comment. Yeah!
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? There was a problem hiding this comment. (Oh, I understood where we use it as a bearer for There was a problem hiding this comment. Yea this class is used internally and could also be used as an easy migration from sendEventWithName. Other events should just implement RCTEvent |
||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| */ | ||
|
|
||
| #import "RCTComponentEvent.h" | ||
|
|
||
| #import "RCTAssert.h" | ||
|
|
||
| @implementation RCTComponentEvent | ||
| { | ||
| NSArray *_arguments; | ||
| } | ||
|
|
||
| @synthesize eventName = _eventName; | ||
| @synthesize viewTag = _viewTag; | ||
|
|
||
| - (instancetype)initWithName:(NSString *)name body:(NSDictionary *)body | ||
| { | ||
| if (self = [super init]) { | ||
| NSNumber *target = body[@"target"]; | ||
|
There was a problem hiding this comment. Why should we extract target from body? Should we pass it directly? There was a problem hiding this comment. 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. There was a problem hiding this comment. Mmm... So, it means that our API reflects details of internal implementation? Does not sound cool. No? There was a problem hiding this comment. We could probably pass target as a parameter and add it to the body in the init. There was a problem hiding this comment. 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. There was a problem hiding this comment. I thought the extra copy to make the NSDictionary mutable would be bad but we actually did it already here https://github.com/facebook/react-native/pull/15894/files#diff-b9fc78351b7b976a36356ef6f08c1f79L117. So the main difference performance wise is the extra allocation of RCTEvent |
||
| name = RCTNormalizeInputEventName(name); | ||
|
|
||
|
There was a problem hiding this comment. Yea that would be better (just copied that code :)) |
||
| if (RCT_DEBUG) { | ||
| RCTAssert([target isKindOfClass:[NSNumber class]], | ||
| @"Event body dictionary must include a 'target' property containing a React tag"); | ||
| } | ||
|
|
||
| _eventName = name; | ||
| _viewTag = target; | ||
| _arguments = @[target, name, body]; | ||
| } | ||
| return self; | ||
| } | ||
|
|
||
| RCT_NOT_IMPLEMENTED(- (instancetype)init) | ||
|
|
||
| - (NSArray *)arguments | ||
| { | ||
| return _arguments; | ||
| } | ||
|
|
||
| - (BOOL)canCoalesce | ||
| { | ||
| return NO; | ||
| } | ||
|
|
||
| + (NSString *)moduleDotMethod | ||
| { | ||
| return @"RCTEventEmitter.receiveEvent"; | ||
| } | ||
|
|
||
| @end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we discribe here that it is general purpose event? Should/can it be subclasses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be subclassed but just reimplementing RCTEvent is probably better especially now that a bunch of methods are optional.