Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
33 changes: 31 additions & 2 deletions RNTester/RNTesterUnitTests/RCTEventDispatcherTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ - (void)dispatchBlock:(dispatch_block_t)block
@end

@implementation RCTDummyBridge
- (void)dispatchBlock:(dispatch_block_t)block
queue:(dispatch_queue_t)queue
- (void)dispatchBlock:(__unused dispatch_block_t)block
queue:(__unused dispatch_queue_t)queue
{}
@end

Expand Down Expand Up @@ -220,6 +220,35 @@ - (void)testDifferentEventTypesDontCoalesce
[_bridge verify];
}

- (void)testDifferentViewTagsDontCoalesce
{
RCTTestEvent *firstEvent = [[RCTTestEvent alloc] initWithViewTag:@(1)
eventName:_eventName
body:_body
coalescingKey:0];
RCTTestEvent *secondEvent = [[RCTTestEvent alloc] initWithViewTag:@(2)
eventName:_eventName
body:_body
coalescingKey:0];

__block dispatch_block_t eventsEmittingBlock;
[[_bridge expect] dispatchBlock:[OCMArg checkWithBlock:^(dispatch_block_t block) {
eventsEmittingBlock = block;
return YES;
}] queue:RCTJSThread];
[[_bridge expect] enqueueJSCall:[[firstEvent class] moduleDotMethod]
args:[firstEvent arguments]];
[[_bridge expect] enqueueJSCall:[[secondEvent class] moduleDotMethod]
args:[secondEvent arguments]];


[_eventDispatcher sendEvent:firstEvent];
[_eventDispatcher sendEvent:secondEvent];
eventsEmittingBlock();

[_bridge verify];
}

- (void)testSameEventTypesWithDifferentCoalesceKeysDontCoalesce
{
NSString *eventName = RCTNormalizeInputEventName(@"firstEvent");
Expand Down
19 changes: 19 additions & 0 deletions React/Base/RCTComponentEvent.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* Copyright (c) Facebook, Inc. and its affilities.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import <React/RCTEventDispatcher.h>

/**
* Generic untyped event for Components. Used internally by RCTDirectEventBlock and
* RCTBubblingEventBlock, for other use cases prefer using a class that implements
* RCTEvent to have a type safe way to initialize it.
*/
@interface RCTComponentEvent : NSObject<RCTEvent>
janicduplessis marked this conversation as resolved.
Show resolved Hide resolved

- (instancetype)initWithName:(NSString *)name viewTag:(NSNumber *)viewTag body:(NSDictionary *)body;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add some nonnull attributes to prevent crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@end
50 changes: 50 additions & 0 deletions React/Base/RCTComponentEvent.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* Copyright (c) Facebook, Inc. and its affilities.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#import "RCTComponentEvent.h"

#import "RCTAssert.h"

@implementation RCTComponentEvent
{
NSArray *_arguments;
}

@synthesize eventName = _eventName;
@synthesize viewTag = _viewTag;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@bvaughn bvaughn Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mutableBody[@"target"] = viewTag;

_eventName = RCTNormalizeInputEventName(name);
_viewTag = viewTag;
_arguments = @[_viewTag, _eventName, mutableBody];
}
return self;
}

RCT_NOT_IMPLEMENTED(- (instancetype)init)

- (NSArray *)arguments
{
return _arguments;
}

- (BOOL)canCoalesce
{
return NO;
}

+ (NSString *)moduleDotMethod
{
return @"RCTEventEmitter.receiveEvent";
}

@end
29 changes: 18 additions & 11 deletions React/Base/RCTEventDispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,31 @@ RCT_EXTERN NSString *RCTNormalizeInputEventName(NSString *eventName);
@protocol RCTEvent <NSObject>
@required

@property (nonatomic, strong, readonly) NSNumber *viewTag;
@property (nonatomic, copy, readonly) NSString *eventName;
@property (nonatomic, assign, readonly) uint16_t coalescingKey;

- (BOOL)canCoalesce;
- (id<RCTEvent>)coalesceWithEvent:(id<RCTEvent>)newEvent;

// used directly for doing a JS call
/** used directly for doing a JS call */
+ (NSString *)moduleDotMethod;
// must contain only JSON compatible values

/** must contain only JSON compatible values */
- (NSArray *)arguments;

@optional

/**
* 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
* returns YES.
*/
@property (nonatomic, assign, readonly) uint16_t coalescingKey;
- (id<RCTEvent>)coalesceWithEvent:(id<RCTEvent>)newEvent;

@end

/**
Expand Down Expand Up @@ -81,12 +94,6 @@ __deprecated_msg("Subclass RCTEventEmitter instead");
- (void)sendDeviceEventWithName:(NSString *)name body:(id)body
__deprecated_msg("Subclass RCTEventEmitter instead");

/**
* Deprecated, do not use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
- (void)sendInputEventWithName:(NSString *)name body:(NSDictionary *)body
__deprecated_msg("Use RCTDirectEventBlock or RCTBubblingEventBlock instead");

/**
* Send a text input/focus event. For internal use only.
*/
Expand Down
85 changes: 38 additions & 47 deletions React/Base/RCTEventDispatcher.m
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
#import "RCTAssert.h"
#import "RCTBridge.h"
#import "RCTBridge+Private.h"
#import "RCTUtils.h"
#import "RCTComponentEvent.h"
#import "RCTProfile.h"
#import "RCTUtils.h"

const NSInteger RCTTextUpdateLagWarningThreshold = 3;

Expand All @@ -29,7 +30,7 @@
static NSNumber *RCTGetEventID(id<RCTEvent> event)
{
return @(
event.viewTag.intValue |
([event respondsToSelector:@selector(viewTag)] ? event.viewTag.intValue : 0) |
(((uint64_t)event.eventName.hash & 0xFFFF) << 32) |
(((uint64_t)event.coalescingKey) << 48)
);
Expand Down Expand Up @@ -79,20 +80,6 @@ - (void)sendDeviceEventWithName:(NSString *)name body:(id)body
completion:NULL];
}

- (void)sendInputEventWithName:(NSString *)name body:(NSDictionary *)body
{
if (RCT_DEBUG) {
RCTAssert([body[@"target"] isKindOfClass:[NSNumber class]],
@"Event body dictionary must include a 'target' property containing a React tag");
}

name = RCTNormalizeInputEventName(name);
[_bridge enqueueJSCall:@"RCTEventEmitter"
method:@"receiveEvent"
args:body ? @[body[@"target"], name, body] : @[body[@"target"], name]
completion:NULL];
}

- (void)sendTextEventWithType:(RCTTextEventType)type
reactTag:(NSNumber *)reactTag
text:(NSString *)text
Expand All @@ -110,7 +97,6 @@ - (void)sendTextEventWithType:(RCTTextEventType)type

NSMutableDictionary *body = [[NSMutableDictionary alloc] initWithDictionary:@{
@"eventCount": @(eventCount),
@"target": reactTag
}];

if (text) {
Expand All @@ -134,10 +120,10 @@ - (void)sendTextEventWithType:(RCTTextEventType)type
body[@"key"] = key;
}

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
[self sendInputEventWithName:events[type] body:body];
#pragma clang diagnostic pop
RCTComponentEvent *event = [[RCTComponentEvent alloc] initWithName:events[type]
viewTag:reactTag
body:body];
[self sendEvent:event];
}

- (void)sendEvent:(id<RCTEvent>)event
Expand All @@ -149,34 +135,39 @@ - (void)sendEvent:(id<RCTEvent>)event
}

[_observersLock unlock];

[_eventQueueLock lock];

NSNumber *eventID = RCTGetEventID(event);

id<RCTEvent> previousEvent = _events[eventID];
if (previousEvent) {
RCTAssert([event canCoalesce], @"Got event %@ which cannot be coalesced, but has the same eventID %@ as the previous event %@", event, eventID, previousEvent);
event = [previousEvent coalesceWithEvent:event];

if (event.canCoalesce) {
[_eventQueueLock lock];

NSNumber *eventID = RCTGetEventID(event);

id<RCTEvent> previousEvent = _events[eventID];
if (previousEvent) {
event = [previousEvent coalesceWithEvent:event];
janicduplessis marked this conversation as resolved.
Show resolved Hide resolved
} else {
[_eventQueue addObject:eventID];
}
_events[eventID] = event;

BOOL scheduleEventsDispatch = NO;
if (!_eventsDispatchScheduled) {
_eventsDispatchScheduled = YES;
scheduleEventsDispatch = YES;
}

// We have to release the lock before dispatching block with events,
// since dispatchBlock: can be executed synchronously on the same queue.
// (This is happening when chrome debugging is turned on.)
[_eventQueueLock unlock];

if (scheduleEventsDispatch) {
[_bridge dispatchBlock:^{
[self flushEventsQueue];
} queue:RCTJSThread];
}
} else {
[_eventQueue addObject:eventID];
}
_events[eventID] = event;

BOOL scheduleEventsDispatch = NO;
if (!_eventsDispatchScheduled) {
_eventsDispatchScheduled = YES;
scheduleEventsDispatch = YES;
}

// We have to release the lock before dispatching block with events,
// since dispatchBlock: can be executed synchronously on the same queue.
// (This is happening when chrome debugging is turned on.)
[_eventQueueLock unlock];

if (scheduleEventsDispatch) {
[_bridge dispatchBlock:^{
[self flushEventsQueue];
[self dispatchEvent:event];
} queue:RCTJSThread];
}
}
Expand Down
15 changes: 15 additions & 0 deletions React/React.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@
14F7A0F01BDA714B003C6C10 /* RCTFPSGraph.m in Sources */ = {isa = PBXBuildFile; fileRef = 14F7A0EF1BDA714B003C6C10 /* RCTFPSGraph.m */; };
191E3EBE1C29D9AF00C180A6 /* RCTRefreshControlManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 191E3EBD1C29D9AF00C180A6 /* RCTRefreshControlManager.m */; };
191E3EC11C29DC3800C180A6 /* RCTRefreshControl.m in Sources */ = {isa = PBXBuildFile; fileRef = 191E3EC01C29DC3800C180A6 /* RCTRefreshControl.m */; };
1968A25F1F67275300EB3D1D /* RCTComponentEvent.h in Headers */ = {isa = PBXBuildFile; fileRef = 1968A25D1F67275300EB3D1D /* RCTComponentEvent.h */; };
1968A2601F67275300EB3D1D /* RCTComponentEvent.h in Headers */ = {isa = PBXBuildFile; fileRef = 1968A25D1F67275300EB3D1D /* RCTComponentEvent.h */; };
1968A2611F67275300EB3D1D /* RCTComponentEvent.m in Sources */ = {isa = PBXBuildFile; fileRef = 1968A25E1F67275300EB3D1D /* RCTComponentEvent.m */; };
1968A2621F67275300EB3D1D /* RCTComponentEvent.m in Sources */ = {isa = PBXBuildFile; fileRef = 1968A25E1F67275300EB3D1D /* RCTComponentEvent.m */; };
199B8A6F1F44DB16005DEF67 /* RCTVersion.h in Headers */ = {isa = PBXBuildFile; fileRef = 199B8A6E1F44DB16005DEF67 /* RCTVersion.h */; };
199B8A761F44DEDA005DEF67 /* RCTVersion.h in Headers */ = {isa = PBXBuildFile; fileRef = 199B8A6E1F44DB16005DEF67 /* RCTVersion.h */; };
19F61BFA1E8495CD00571D81 /* bignum-dtoa.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = 139D7E3A1E25C5A300323FB7 /* bignum-dtoa.h */; };
Expand Down Expand Up @@ -2021,6 +2025,8 @@
191E3EBD1C29D9AF00C180A6 /* RCTRefreshControlManager.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTRefreshControlManager.m; sourceTree = "<group>"; };
191E3EBF1C29DC3800C180A6 /* RCTRefreshControl.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTRefreshControl.h; sourceTree = "<group>"; };
191E3EC01C29DC3800C180A6 /* RCTRefreshControl.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTRefreshControl.m; sourceTree = "<group>"; };
1968A25D1F67275300EB3D1D /* RCTComponentEvent.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTComponentEvent.h; sourceTree = "<group>"; };
1968A25E1F67275300EB3D1D /* RCTComponentEvent.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = RCTComponentEvent.m; sourceTree = "<group>"; };
199B8A6E1F44DB16005DEF67 /* RCTVersion.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RCTVersion.h; sourceTree = "<group>"; };
27B958731E57587D0096647A /* JSBigString.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSBigString.cpp; sourceTree = "<group>"; };
2D2A28131D9B038B00D4039D /* libReact.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libReact.a; sourceTree = BUILT_PRODUCTS_DIR; };
Expand Down Expand Up @@ -2930,6 +2936,8 @@
830213F31A654E0800B993E6 /* RCTBridgeModule.h */,
68EFE4EC1CF6EB3000A1DE13 /* RCTBundleURLProvider.h */,
68EFE4ED1CF6EB3900A1DE13 /* RCTBundleURLProvider.m */,
1968A25D1F67275300EB3D1D /* RCTComponentEvent.h */,
1968A25E1F67275300EB3D1D /* RCTComponentEvent.m */,
83CBBACA1A6023D300E9B192 /* RCTConvert.h */,
83CBBACB1A6023D300E9B192 /* RCTConvert.m */,
C60128A91F3D1258009DF9FF /* RCTCxxConvert.h */,
Expand Down Expand Up @@ -3242,6 +3250,10 @@
594F0A331FD23228007FBE96 /* RCTSurfaceHostingView.h in Headers */,
130443DD1E401AF500D93A67 /* RCTConvert+Transform.h in Headers */,
134D63C41F1FEC65008872B5 /* RCTCxxBridgeDelegate.h in Headers */,
3D302F801DF828F800D6DDAE /* RCTNavItem.h in Headers */,
1968A2601F67275300EB3D1D /* RCTComponentEvent.h in Headers */,
3D302F811DF828F800D6DDAE /* RCTNavItemManager.h in Headers */,
135A9C061E7B0F7800587AEB /* RCTJSCHelpers.h in Headers */,
3D302F841DF828F800D6DDAE /* RCTPointerEvents.h in Headers */,
59EB6DBC1EBD6FC90072A5E7 /* RCTUIManagerObserverCoordinator.h in Headers */,
657734941EE8356100A0E9EA /* RCTInspectorPackagerConnection.h in Headers */,
Expand Down Expand Up @@ -3415,6 +3427,7 @@
599FAA361FB274980058CCF6 /* RCTSurface.h in Headers */,
3D80DA231DF820620028D040 /* RCTBridgeDelegate.h in Headers */,
3D80DA241DF820620028D040 /* RCTBridgeMethod.h in Headers */,
1968A25F1F67275300EB3D1D /* RCTComponentEvent.h in Headers */,
3D7BFD151EA8E351008DFB7A /* RCTPackagerClient.h in Headers */,
3D80DA251DF820620028D040 /* RCTBridgeModule.h in Headers */,
3D80DA261DF820620028D040 /* RCTBundleURLProvider.h in Headers */,
Expand Down Expand Up @@ -4234,6 +4247,7 @@
2D3B5EE41D9B09BB00451313 /* RCTSegmentedControlManager.m in Sources */,
59EDBCB81FDF4E0C003573DE /* RCTScrollView.m in Sources */,
13134C9F1E296B2A00B9F3CB /* RCTCxxModule.mm in Sources */,
1968A2621F67275300EB3D1D /* RCTComponentEvent.m in Sources */,
2D3B5EE31D9B09B700451313 /* RCTSegmentedControl.m in Sources */,
130443A41E3FEAC600D93A67 /* RCTFollyConvert.mm in Sources */,
3D7BFD201EA8E351008DFB7A /* RCTPackagerConnection.mm in Sources */,
Expand Down Expand Up @@ -4489,6 +4503,7 @@
59283CA01FD67321000EAAB9 /* RCTSurfaceStage.m in Sources */,
13513F3C1B1F43F400FCE529 /* RCTProgressViewManager.m in Sources */,
14F7A0F01BDA714B003C6C10 /* RCTFPSGraph.m in Sources */,
1968A2611F67275300EB3D1D /* RCTComponentEvent.m in Sources */,
3D7BFD171EA8E351008DFB7A /* RCTPackagerClient.m in Sources */,
14F3620D1AABD06A001CE568 /* RCTSwitch.m in Sources */,
13134C8E1E296B2A00B9F3CB /* RCTMessageThread.mm in Sources */,
Expand Down
11 changes: 5 additions & 6 deletions React/Views/RCTComponentData.m
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#import "RCTBridge.h"
#import "RCTBridgeModule.h"
#import "RCTComponentEvent.h"
#import "RCTConvert.h"
#import "RCTParserUtils.h"
#import "RCTShadowView.h"
Expand Down Expand Up @@ -114,12 +115,10 @@ static RCTPropBlock createEventSetter(NSString *propName, SEL setter, RCTBridge
return;
}

NSMutableDictionary *mutableEvent = [NSMutableDictionary dictionaryWithDictionary:event];
mutableEvent[@"target"] = strongTarget.reactTag;
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
[weakBridge.eventDispatcher sendInputEventWithName:RCTNormalizeInputEventName(propName) body:mutableEvent];
#pragma clang diagnostic pop
RCTComponentEvent *componentEvent = [[RCTComponentEvent alloc] initWithName:propName
viewTag:strongTarget.reactTag
body:event];
[weakBridge.eventDispatcher sendEvent:componentEvent];
};
}
((void (*)(id, SEL, id))objc_msgSend)(target, setter, eventHandler);
Expand Down