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

[Fabric] Extract Fabric event handlers from canonical props #13024

Merged
merged 1 commit into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-native-renderer/src/ReactFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import ReactVersion from 'shared/ReactVersion';

import NativeMethodsMixin from './NativeMethodsMixin';
import ReactNativeComponent from './ReactNativeComponent';
import * as ReactNativeComponentTree from './ReactNativeComponentTree';
import * as ReactFabricComponentTree from './ReactFabricComponentTree';
import {getInspectorDataForViewTag} from './ReactNativeFiberInspector';

import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
Expand Down Expand Up @@ -120,7 +120,7 @@ const ReactFabric: ReactFabricType = {
};

ReactFabricRenderer.injectIntoDevTools({
findFiberByHostInstance: ReactNativeComponentTree.getClosestInstanceFromNode,
findFiberByHostInstance: ReactFabricComponentTree.getClosestInstanceFromNode,
getInspectorDataForViewTag: getInspectorDataForViewTag,
bundleType: __DEV__ ? 1 : 0,
version: ReactVersion,
Expand Down
28 changes: 28 additions & 0 deletions packages/react-native-renderer/src/ReactFabricComponentTree.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/**
* Copyright (c) 2013-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import invariant from 'fbjs/lib/invariant';

function getInstanceFromInstance(instanceHandle) {
return instanceHandle;
}

function getTagFromInstance(inst) {
let tag = inst.stateNode.canonical._nativeTag;
invariant(tag, 'All native instances should have a tag.');
return tag;
}

export {
getInstanceFromInstance as getClosestInstanceFromNode,
getInstanceFromInstance as getInstanceFromNode,
getTagFromInstance as getNodeFromInstance,
};

export function getFiberCurrentPropsFromNode(inst) {
return inst.canonical.currentProps;
}
2 changes: 2 additions & 0 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ export function prepareUpdate(
);
// TODO: If the event handlers have changed, we need to update the current props
// in the commit phase but there is no host config hook to do it yet.
// So instead we hack it by updating it in the render phase.
instance.canonical.currentProps = newProps;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is sufficient to fix the crashes? How important is the follow-up PR to add a commit phase hook?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't matter for sync mode but will be important eventually.

I'm considering using the memoizedProps solution to avoid having to add a commit phase in JS. Currently the commit phase is entire in native.

return updatePayload;
}

Expand Down
14 changes: 3 additions & 11 deletions packages/react-native-renderer/src/ReactFabricInjection.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,7 @@

import './ReactNativeInjectionShared';

// TODO: The event emitter registration is interfering with the existing
// ReactNative renderer. So disable it for Fabric for now.
import * as ReactFabricComponentTree from './ReactFabricComponentTree';
import * as EventPluginUtils from 'events/EventPluginUtils';

// import * as ReactNativeEventEmitter from './ReactNativeEventEmitter';

// Module provided by RN:
// import RCTEventEmitter from 'RCTEventEmitter';

/**
* Register the event emitter with the native bridge
*/
// RCTEventEmitter.register(ReactNativeEventEmitter);
EventPluginUtils.injection.injectComponentTree(ReactFabricComponentTree);
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,7 @@ export function uncacheFiberNode(tag) {
}

function getInstanceFromTag(tag) {
if (typeof tag === 'number') {
return instanceCache[tag] || null;
} else {
// Fabric will invoke event emitters on a direct fiber reference
return tag;
}
return instanceCache[tag] || null;
}

function getTagFromInstance(inst) {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-native-renderer/src/ReactNativeInjection.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

import './ReactNativeInjectionShared';

import * as ReactNativeComponentTree from './ReactNativeComponentTree';
import * as EventPluginUtils from 'events/EventPluginUtils';
import * as ReactNativeEventEmitter from './ReactNativeEventEmitter';

// Module provided by RN:
Expand All @@ -18,3 +20,5 @@ import RCTEventEmitter from 'RCTEventEmitter';
* Register the event emitter with the native bridge
*/
RCTEventEmitter.register(ReactNativeEventEmitter);

EventPluginUtils.injection.injectComponentTree(ReactNativeComponentTree);
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,16 @@
import 'InitializeCore';

import * as EventPluginHub from 'events/EventPluginHub';
import * as EventPluginUtils from 'events/EventPluginUtils';
import ResponderEventPlugin from 'events/ResponderEventPlugin';

import ReactNativeBridgeEventPlugin from './ReactNativeBridgeEventPlugin';
import * as ReactNativeComponentTree from './ReactNativeComponentTree';
import ReactNativeEventPluginOrder from './ReactNativeEventPluginOrder';
import ReactNativeGlobalResponderHandler from './ReactNativeGlobalResponderHandler';

/**
* Inject module for resolving DOM hierarchy and plugin ordering.
*/
EventPluginHub.injection.injectEventPluginOrder(ReactNativeEventPluginOrder);
EventPluginUtils.injection.injectComponentTree(ReactNativeComponentTree);

ResponderEventPlugin.injection.injectGlobalResponderHandler(
ReactNativeGlobalResponderHandler,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -352,4 +352,49 @@ describe('ReactFabric', () => {
11,
);
});

it('dispatches events to the last committed props', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {},
uiViewClassName: 'RCTView',
directEventTypes: {
topTouchStart: {
registrationName: 'onTouchStart',
},
},
}));

const touchStart = jest.fn();
const touchStart2 = jest.fn();

ReactFabric.render(<View onTouchStart={touchStart} />, 11);

expect(FabricUIManager.createNode.mock.calls.length).toBe(1);
expect(FabricUIManager.registerEventHandler.mock.calls.length).toBe(1);

let [, , , , instanceHandle] = FabricUIManager.createNode.mock.calls[0];
let [dispatchEvent] = FabricUIManager.registerEventHandler.mock.calls[0];

let touchEvent = {
touches: [],
changedTouches: [],
};

expect(touchStart).not.toBeCalled();

dispatchEvent(instanceHandle, 'topTouchStart', touchEvent);

expect(touchStart).toBeCalled();
expect(touchStart2).not.toBeCalled();

ReactFabric.render(<View onTouchStart={touchStart2} />, 11);

// Intentionally dispatch to the same instanceHandle again.
dispatchEvent(instanceHandle, 'topTouchStart', touchEvent);

// The current semantics dictate that we always dispatch to the last committed
// props even though the actual scheduling of the event could have happened earlier.
// This could change in the future.
expect(touchStart2).toBeCalled();
});
});