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

Adding ReactNative.setNativeProps that takes a ref #14907

Merged
merged 5 commits into from
Feb 21, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/react-native-renderer/src/ReactFabric.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import NativeMethodsMixin from './NativeMethodsMixin';
import ReactNativeComponent from './ReactNativeComponent';
import {getClosestInstanceFromNode} from './ReactFabricComponentTree';
import {getInspectorDataForViewTag} from './ReactNativeFiberInspector';
import {setNativeProps} from './ReactNativeSetNativeProps';

import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
Expand Down Expand Up @@ -104,6 +105,8 @@ const ReactFabric: ReactFabricType = {

findNodeHandle,

setNativeProps,

render(element: React$Element<any>, containerTag: any, callback: ?Function) {
let root = roots.get(containerTag);

Expand Down
3 changes: 3 additions & 0 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import NativeMethodsMixin from './NativeMethodsMixin';
import ReactNativeComponent from './ReactNativeComponent';
import {getClosestInstanceFromNode} from './ReactNativeComponentTree';
import {getInspectorDataForViewTag} from './ReactNativeFiberInspector';
import {setNativeProps} from './ReactNativeSetNativeProps';

import ReactSharedInternals from 'shared/ReactSharedInternals';
import getComponentName from 'shared/getComponentName';
Expand Down Expand Up @@ -116,6 +117,8 @@ const ReactNativeRenderer: ReactNativeType = {

findNodeHandle,

setNativeProps,

render(element: React$Element<any>, containerTag: any, callback: ?Function) {
let root = roots.get(containerTag);

Expand Down
43 changes: 43 additions & 0 deletions packages/react-native-renderer/src/ReactNativeSetNativeProps.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you’re going to add more of these, can we name it something more general so we don’t need a file per method?

* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
*/

import {create} from './ReactNativeAttributePayload';
import {warnForStyleProps} from './NativeMethodsMixinUtils';

import warningWithoutStack from 'shared/warningWithoutStack';

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

export function setNativeProps(handle: any, nativeProps: Object) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better flow type I should be using for handle?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We used to pretend that native host instances were ReactNativeComponent. That’s not technically true because they’re not instanceof.

With forwardRef, we get the inner type. I would’ve expected that you have to supply the ref type to Flow. What does Flow think the type of a ref is?

Ideally it would be an opaque type which will help catch anyone trying to call something else on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, seems like a problem for another PR at another time. :)

if (handle._nativeTag == null) {
warningWithoutStack(
handle._nativeTag != null,
"setNativeProps was called on a ref that isn't a " +
'native component. Use React.forwardRef to get access to the underlying native component',
);
return;
}

if (__DEV__) {
warnForStyleProps(nativeProps, handle.viewConfig.validAttributes);
}

const updatePayload = create(nativeProps, handle.viewConfig.validAttributes);
// Avoid the overhead of bridge calls if there's no update.
// This is an expensive no-op for Android, and causes an unnecessary
// view invalidation for certain components (eg RCTTextInput) on iOS.
if (updatePayload != null) {
UIManager.updateView(
handle._nativeTag,
handle.viewConfig.uiViewClassName,
updatePayload,
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ describe('ReactFabric', () => {
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
});

it('should not call UIManager.updateView from setNativeProps for properties that have not changed', () => {
it('should not call UIManager.updateView from ref.setNativeProps for properties that have not changed', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
Expand Down Expand Up @@ -214,6 +214,90 @@ describe('ReactFabric', () => {
});
});

it('should be able to setNativeProps on native refs', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

UIManager.updateView.mockReset();

let viewRef;
ReactFabric.render(
<View
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>,
11,
);

expect(UIManager.updateView).not.toBeCalled();
ReactFabric.setNativeProps(viewRef, {foo: 'baz'});
expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
expect.any(Number),
'RCTView',
{foo: 'baz'},
);
});

it('should warn and no-op if calling setNativeProps on non native refs', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

class BasicClass extends React.Component {
render() {
return <React.Fragment />;
}
}

class Subclass extends ReactFabric.NativeComponent {
render() {
return <View />;
}
}

const CreateClass = createReactClass({
mixins: [NativeMethodsMixin],
render: () => {
return <View />;
},
});

[BasicClass, Subclass, CreateClass].forEach(Component => {
UIManager.updateView.mockReset();

let viewRef;
ReactFabric.render(
<Component
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>,
11,
);

expect(UIManager.updateView).not.toBeCalled();
expect(() => {
ReactFabric.setNativeProps(viewRef, {foo: 'baz'});
}).toWarnDev(
[
"Warning: setNativeProps was called on a ref that isn't a " +
'native component. Use React.forwardRef to get access ' +
'to the underlying native component',
],
{withoutStack: true},
);

expect(UIManager.updateView).not.toBeCalled();
});
});

it('returns the correct instance and calls it in the callback', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@
let React;
let ReactFabric;
let ReactNative;
let UIManager;
let createReactNativeComponentClass;

describe('ReactFabric', () => {
beforeEach(() => {
jest.resetModules();
ReactNative = require('react-native-renderer');
UIManager = require('UIManager');
jest.resetModules();
jest.mock('shared/ReactFeatureFlags', () =>
require('shared/forks/ReactFeatureFlags.native-oss'),
Expand Down Expand Up @@ -49,4 +51,25 @@ describe('ReactFabric', () => {
let handle = ReactNative.findNodeHandle(ref.current);
expect(handle).toBe(2);
});

it('sets native props with setNativeProps on Fabric nodes with the RN renderer', () => {
UIManager.updateView.mockReset();
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {title: true},
uiViewClassName: 'RCTView',
}));

let ref = React.createRef();

ReactFabric.render(<View title="bar" ref={ref} />, 11);
expect(UIManager.updateView).not.toBeCalled();
ReactNative.setNativeProps(ref.current, {title: 'baz'});
expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
expect.any(Number),
'RCTView',
{title: 'baz'},
);
});

});
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,90 @@ describe('ReactNative', () => {
});
});

it('should be able to setNativeProps on native refs', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

UIManager.updateView.mockReset();

let viewRef;
ReactNative.render(
<View
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>,
11,
);

expect(UIManager.updateView).not.toBeCalled();
ReactNative.setNativeProps(viewRef, {foo: 'baz'});
expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
expect.any(Number),
'RCTView',
{foo: 'baz'},
);
});

it('should warn and no-op if calling setNativeProps on non native refs', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
}));

class BasicClass extends React.Component {
render() {
return <React.Fragment />;
}
}

class Subclass extends ReactNative.NativeComponent {
render() {
return <View />;
}
}

const CreateClass = createReactClass({
mixins: [NativeMethodsMixin],
render: () => {
return <View />;
},
});

[BasicClass, Subclass, CreateClass].forEach(Component => {
UIManager.updateView.mockReset();

let viewRef;
ReactNative.render(
<Component
foo="bar"
ref={ref => {
viewRef = ref;
}}
/>,
11,
);

expect(UIManager.updateView).not.toBeCalled();
expect(() => {
ReactNative.setNativeProps(viewRef, {foo: 'baz'});
}).toWarnDev(
[
"Warning: setNativeProps was called on a ref that isn't a " +
'native component. Use React.forwardRef to get access ' +
'to the underlying native component',
],
{withoutStack: true},
);

expect(UIManager.updateView).not.toBeCalled();
});
});

it('returns the correct instance and calls it in the callback', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
Expand Down