From 574457114097aabe77fe4cd7b5ed93b8cd3d6795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 6 Oct 2017 19:08:22 -0700 Subject: [PATCH] [RT] More predictable things (#11139) * Inject the right event emitter Previously this was injecting the ReactNativeEventEmitter even though we want the ReactNativeRTEventEmitter. RCTEventEmitter is also just an abstraction around BatchedBridge that registers a single name. We can't register more than one with it. Removed that abstraction for now so we don't have to add it back into the RN repo. * Unify appendChildToDetachedParent and appendChild, separate root Merge appendChildToDetachedParent and appendChild. We don't need the distinction. We do however need a separate notion for the root container. Calling this `appendChildToContext` since Context has a meaning here. * Add a simple shallow comparison so that we don't send updates for equal props This still sends all props if any differs. Not sure we'll want that but I think so. * Add BatchedBridge to bundle externals * Lint --- flow/react-native-host-hooks.js | 15 +++++--- scripts/rollup/bundles.js | 2 +- src/__mocks__/BatchedBridge.js | 14 ++++++++ src/__mocks__/RTManager.js | 6 ++-- .../native-rt/ReactNativeRTEventEmitter.js | 6 +++- .../native-rt/ReactNativeRTFiberEntry.js | 8 +---- .../native-rt/ReactNativeRTFiberRenderer.js | 34 ++++++++++++++++--- .../native-rt/__tests__/ReactNativeRT-test.js | 16 +++++++-- 8 files changed, 78 insertions(+), 23 deletions(-) create mode 100644 src/__mocks__/BatchedBridge.js diff --git a/flow/react-native-host-hooks.js b/flow/react-native-host-hooks.js index 9b225116a6..7a7119c9e1 100644 --- a/flow/react-native-host-hooks.js +++ b/flow/react-native-host-hooks.js @@ -93,13 +93,13 @@ declare module 'RTManager' { classType : string, props : ?Object, ) : void; - declare function appendChildToDetachedParent( - parentTag : number, - childTag : number, - ) : void; declare function beginUpdates() : void; + declare function appendChildToContext( + contextTag : number, + childTag : number, + ) : void; declare function appendChild( parentTag : number, childTag : number, @@ -118,3 +118,10 @@ declare module 'RTManager' { declare function completeUpdates() : void; } + +declare module 'BatchedBridge' { + declare function registerCallableModule( + name : string, + module : Object, + ) : void; +} diff --git a/scripts/rollup/bundles.js b/scripts/rollup/bundles.js index bf28857300..a8e536eb11 100644 --- a/scripts/rollup/bundles.js +++ b/scripts/rollup/bundles.js @@ -302,7 +302,7 @@ const bundles = [ 'ExceptionsManager', 'InitializeCore', 'Platform', - 'RCTEventEmitter', + 'BatchedBridge', 'RTManager', 'prop-types/checkPropTypes', ], diff --git a/src/__mocks__/BatchedBridge.js b/src/__mocks__/BatchedBridge.js new file mode 100644 index 0000000000..b46e3dd4da --- /dev/null +++ b/src/__mocks__/BatchedBridge.js @@ -0,0 +1,14 @@ +/** + * 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. + */ + +'use strict'; + +var BatchedBridge = { + registerCallableModule: jest.fn(), +}; + +module.exports = BatchedBridge; diff --git a/src/__mocks__/RTManager.js b/src/__mocks__/RTManager.js index f4bb15fbca..860b440d84 100644 --- a/src/__mocks__/RTManager.js +++ b/src/__mocks__/RTManager.js @@ -11,11 +11,11 @@ var RCTRTManager = { createNode: jest.fn(function createView(tag, classType, props) {}), - appendChildToDetachedParent: jest.fn(function appendChildToDetachedParent( - parentTag, + beginUpdates: jest.fn(function beginUpdates() {}), + appendChildToContext: jest.fn(function appendChildToContext( + contextTag, childTag, ) {}), - beginUpdates: jest.fn(function beginUpdates() {}), appendChild: jest.fn(function appendChild(parentTag, childTag) {}), prependChild: jest.fn(function prependChild(childTag, beforeTag) {}), deleteChild: jest.fn(function deleteChild(childTag) {}), diff --git a/src/renderers/native-rt/ReactNativeRTEventEmitter.js b/src/renderers/native-rt/ReactNativeRTEventEmitter.js index 3aa19cf2e6..a6855678dc 100644 --- a/src/renderers/native-rt/ReactNativeRTEventEmitter.js +++ b/src/renderers/native-rt/ReactNativeRTEventEmitter.js @@ -11,6 +11,7 @@ var ReactNativeRTComponentTree = require('ReactNativeRTComponentTree'); var ReactGenericBatching = require('ReactGenericBatching'); +var BatchedBridge = require('BatchedBridge'); var ReactNativeRTEventEmitter = { /** @@ -40,4 +41,7 @@ var ReactNativeRTEventEmitter = { }, }; -module.exports = ReactNativeRTEventEmitter; +BatchedBridge.registerCallableModule( + 'RTEventEmitter', + ReactNativeRTEventEmitter, +); diff --git a/src/renderers/native-rt/ReactNativeRTFiberEntry.js b/src/renderers/native-rt/ReactNativeRTFiberEntry.js index 5ef2223893..a367e32336 100644 --- a/src/renderers/native-rt/ReactNativeRTFiberEntry.js +++ b/src/renderers/native-rt/ReactNativeRTFiberEntry.js @@ -32,13 +32,7 @@ import type {ReactNodeList} from 'ReactTypes'; */ require('InitializeCore'); -var RCTEventEmitter = require('RCTEventEmitter'); -var ReactNativeEventEmitter = require('ReactNativeEventEmitter'); - -/** - * Register the event emitter with the native bridge - */ -RCTEventEmitter.register(ReactNativeEventEmitter); +require('ReactNativeRTEventEmitter'); ReactGenericBatching.injection.injectFiberBatchedUpdates( ReactNativeRTFiberRenderer.batchedUpdates, diff --git a/src/renderers/native-rt/ReactNativeRTFiberRenderer.js b/src/renderers/native-rt/ReactNativeRTFiberRenderer.js index 7fe91d5b4e..799ee85c1f 100644 --- a/src/renderers/native-rt/ReactNativeRTFiberRenderer.js +++ b/src/renderers/native-rt/ReactNativeRTFiberRenderer.js @@ -45,6 +45,29 @@ function processProps(instance: number, props: Props): Object { return propsPayload; } +function arePropsEqual(oldProps: Props, newProps: Props): boolean { + var key; + for (key in newProps) { + if (key === 'children') { + // Skip special case. + continue; + } + if (newProps[key] !== oldProps[key]) { + return false; + } + } + for (key in oldProps) { + if (key === 'children') { + // Skip special case. + continue; + } + if (!(key in newProps)) { + return false; + } + } + return true; +} + const NativeRTRenderer = ReactFiberReconciler({ appendChild(parentInstance: Instance, child: Instance | TextInstance): void { RTManager.appendChild(parentInstance, child); @@ -54,14 +77,14 @@ const NativeRTRenderer = ReactFiberReconciler({ parentInstance: Container, child: Instance | TextInstance, ): void { - RTManager.appendChild(parentInstance, child); + RTManager.appendChildToContext(parentInstance, child); }, appendInitialChild( parentInstance: Instance, child: Instance | TextInstance, ): void { - RTManager.appendChildToDetachedParent(parentInstance, child); + RTManager.appendChild(parentInstance, child); }, commitTextUpdate( @@ -90,7 +113,7 @@ const NativeRTRenderer = ReactFiberReconciler({ internalInstanceHandle: Object, ): void { updateFiberProps(instance, newProps); - RTManager.updateNode(instance, processProps(instance, newProps)); + RTManager.updateNode(instance, updatePayload); }, createInstance( @@ -165,7 +188,10 @@ const NativeRTRenderer = ReactFiberReconciler({ rootContainerInstance: Container, hostContext: {}, ): null | Object { - return emptyObject; + if (arePropsEqual(oldProps, newProps)) { + return null; + } + return processProps(instance, newProps); }, removeChild(parentInstance: Instance, child: Instance | TextInstance): void { diff --git a/src/renderers/native-rt/__tests__/ReactNativeRT-test.js b/src/renderers/native-rt/__tests__/ReactNativeRT-test.js index 6cf7d713e5..fa23375665 100644 --- a/src/renderers/native-rt/__tests__/ReactNativeRT-test.js +++ b/src/renderers/native-rt/__tests__/ReactNativeRT-test.js @@ -25,7 +25,8 @@ describe('ReactNativeRT', () => { it('should be able to create and render a native component', () => { ReactNativeRT.render(, 1); expect(RTManager.createNode).toBeCalled(); - expect(RTManager.appendChild).toBeCalled(); + expect(RTManager.appendChildToContext).toBeCalled(); + expect(RTManager.appendChild).not.toBeCalled(); expect(RTManager.updateNode).not.toBeCalled(); }); @@ -34,13 +35,22 @@ describe('ReactNativeRT', () => { expect(RTManager.createNode.mock.calls.length).toBe(1); expect(RTManager.createNode).toBeCalledWith(1, 'rt-box', {foo: 'foo'}); - expect(RTManager.appendChild.mock.calls.length).toBe(1); + expect(RTManager.appendChildToContext.mock.calls.length).toBe(1); + expect(RTManager.appendChild).not.toBeCalled(); expect(RTManager.updateNode).not.toBeCalled(); ReactNativeRT.render(, 11); expect(RTManager.createNode.mock.calls.length).toBe(1); - expect(RTManager.appendChild.mock.calls.length).toBe(1); + expect(RTManager.appendChildToContext.mock.calls.length).toBe(1); + expect(RTManager.appendChild).not.toBeCalled(); expect(RTManager.updateNode).toBeCalledWith(1, {foo: 'bar'}); + + ReactNativeRT.render(, 11); + + expect(RTManager.createNode.mock.calls.length).toBe(2); + expect(RTManager.appendChildToContext.mock.calls.length).toBe(1); + expect(RTManager.appendChildToContext.mock.calls.length).toBe(1); + expect(RTManager.updateNode.mock.calls.length).toBe(1); }); });