Skip to content

Make setNativeProps a no-op with Fabric renderer #15094

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.

Sign up for GitHub

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

Merged
bvaughn merged 2 commits into from
Mar 29, 2019
Merged

Make setNativeProps a no-op with Fabric renderer #15094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54f803b
Make setNativeProps a no-op with Fabric renderer
elicwhite Mar 12, 2019
4b4694a
Remove unnecessary __DEV__ check
elicwhite Mar 12, 2019
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
31 changes: 20 additions & 11 deletions packages/react-native-renderer/src/NativeMethodsMixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,6 @@ export default function(
* Manipulation](docs/direct-manipulation.html)).
*/
setNativeProps: function(nativeProps: Object) {
if (__DEV__) {
if (warnAboutDeprecatedSetNativeProps) {
warningWithoutStack(
false,
'Warning: Calling ref.setNativeProps(nativeProps) ' +
'is deprecated and will be removed in a future release. ' +
'Use the setNativeProps export from the react-native package instead.' +
"\n\timport {setNativeProps} from 'react-native';\n\tsetNativeProps(ref, nativeProps);\n",
);
}
}
// Class components don't have viewConfig -> validateAttributes.
// Nor does it make sense to set native props on a non-native component.
// Instead, find the nearest host component and set props on it.
Expand All @@ -156,6 +145,26 @@ export default function(
return;
}

if (maybeInstance.canonical) {
warningWithoutStack(
false,
'Warning: setNativeProps is not currently supported in Fabric',
);
return;
}

if (__DEV__) {
if (warnAboutDeprecatedSetNativeProps) {
warningWithoutStack(
false,
'Warning: Calling ref.setNativeProps(nativeProps) ' +
'is deprecated and will be removed in a future release. ' +
'Use the setNativeProps export from the react-native package instead.' +
"\n\timport {setNativeProps} from 'react-native';\n\tsetNativeProps(ref, nativeProps);\n",
);
}
}

const nativeTag =
maybeInstance._nativeTag || maybeInstance.canonical._nativeTag;
const viewConfig: ReactNativeBaseComponentViewConfig<> =
Expand Down
10 changes: 8 additions & 2 deletions packages/react-native-renderer/src/ReactFabric.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import NativeMethodsMixin from './NativeMethodsMixin';
import ReactNativeComponent from './ReactNativeComponent';
import {getClosestInstanceFromNode} from './ReactFabricComponentTree';
import {getInspectorDataForViewTag} from './ReactNativeFiberInspector';
import {setNativeProps} from './ReactNativeRendererSharedExports';

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

findNodeHandle,

setNativeProps,
setNativeProps(handle: any, nativeProps: Object) {
warningWithoutStack(
false,
'Warning: setNativeProps is not currently supported in Fabric',
);

return;
},

render(element: React$Element<any>, containerTag: any, callback: ?Function) {
let root = roots.get(containerTag);
Expand Down
35 changes: 6 additions & 29 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,13 @@ import type {
ReactNativeBaseComponentViewConfig,
} from './ReactNativeTypes';

import {
mountSafeCallback_NOT_REALLY_SAFE,
warnForStyleProps,
} from './NativeMethodsMixinUtils';
import {mountSafeCallback_NOT_REALLY_SAFE} from './NativeMethodsMixinUtils';
import {create, diff} from './ReactNativeAttributePayload';
import {get as getViewConfigForType} from 'ReactNativeViewConfigRegistry';

import deepFreezeAndThrowOnMutationInDev from 'deepFreezeAndThrowOnMutationInDev';
import invariant from 'shared/invariant';
import warningWithoutStack from 'shared/warningWithoutStack';
import {warnAboutDeprecatedSetNativeProps} from 'shared/ReactFeatureFlags';

import {dispatchEvent} from './ReactFabricEventEmitter';

Expand Down Expand Up @@ -135,31 +131,12 @@ class ReactFabricHostComponent {
}

setNativeProps(nativeProps: Object) {
if (__DEV__) {
if (warnAboutDeprecatedSetNativeProps) {
warningWithoutStack(
false,
'Warning: Calling ref.setNativeProps(nativeProps) ' +
'is deprecated and will be removed in a future release. ' +
'Use the setNativeProps export from the react-native package instead.' +
"\n\timport {setNativeProps} from 'react-native';\n\tsetNativeProps(ref, nativeProps);\n",
);
}
warnForStyleProps(nativeProps, this.viewConfig.validAttributes);
}
warningWithoutStack(
false,
'Warning: setNativeProps is not currently supported in Fabric',
);

const updatePayload = create(nativeProps, this.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(
this._nativeTag,
this.viewConfig.uiViewClassName,
updatePayload,
);
}
return;
}
}

Expand Down
32 changes: 20 additions & 12 deletions packages/react-native-renderer/src/ReactNativeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,18 +135,6 @@ export default function(
* Manipulation](docs/direct-manipulation.html)).
*/
setNativeProps(nativeProps: Object): void {
if (__DEV__) {
if (warnAboutDeprecatedSetNativeProps) {
warningWithoutStack(
false,
'Warning: Calling ref.setNativeProps(nativeProps) ' +
'is deprecated and will be removed in a future release. ' +
'Use the setNativeProps export from the react-native package instead.' +
"\n\timport {setNativeProps} from 'react-native';\n\tsetNativeProps(ref, nativeProps);\n",
);
}
}

// Class components don't have viewConfig -> validateAttributes.
// Nor does it make sense to set native props on a non-native component.
// Instead, find the nearest host component and set props on it.
Expand All @@ -168,6 +156,26 @@ export default function(
return;
}

if (maybeInstance.canonical) {
warningWithoutStack(
false,
'Warning: setNativeProps is not currently supported in Fabric',
);
return;
}

if (__DEV__) {
if (warnAboutDeprecatedSetNativeProps) {
warningWithoutStack(
false,
'Warning: Calling ref.setNativeProps(nativeProps) ' +
'is deprecated and will be removed in a future release. ' +
'Use the setNativeProps export from the react-native package instead.' +
"\n\timport {setNativeProps} from 'react-native';\n\tsetNativeProps(ref, nativeProps);\n",
);
}
}

const nativeTag =
maybeInstance._nativeTag || maybeInstance.canonical._nativeTag;
const viewConfig: ReactNativeBaseComponentViewConfig<> =
Expand Down
46 changes: 16 additions & 30 deletions packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,8 @@ let FabricUIManager;
let StrictMode;
let NativeMethodsMixin;

const SET_NATIVE_PROPS_DEPRECATION_MESSAGE =
'Warning: Calling ref.setNativeProps(nativeProps) ' +
'is deprecated and will be removed in a future release. ' +
'Use the setNativeProps export from the react-native package instead.' +
"\n\timport {setNativeProps} from 'react-native';\n\tsetNativeProps(ref, nativeProps);\n";
const SET_NATIVE_PROPS_NOT_SUPPORTED_MESSAGE =
'Warning: setNativeProps is not currently supported in Fabric';

jest.mock('shared/ReactFeatureFlags', () =>
require('shared/forks/ReactFeatureFlags.native-oss'),
Expand Down Expand Up @@ -176,7 +173,7 @@ describe('ReactFabric', () => {
expect(FabricUIManager.__dumpHierarchyForJestTestsOnly()).toMatchSnapshot();
});

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

expect(() => {
viewRef.setNativeProps({});
}).toWarnDev([SET_NATIVE_PROPS_DEPRECATION_MESSAGE], {
}).toWarnDev([SET_NATIVE_PROPS_NOT_SUPPORTED_MESSAGE], {
withoutStack: true,
});

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

expect(() => {
viewRef.setNativeProps({foo: 'baz'});
}).toWarnDev([SET_NATIVE_PROPS_DEPRECATION_MESSAGE], {
}).toWarnDev([SET_NATIVE_PROPS_NOT_SUPPORTED_MESSAGE], {
withoutStack: true,
});
expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
expect.any(Number),
'RCTView',
{foo: 'baz'},
);
expect(UIManager.updateView).not.toBeCalled();
});
});

it('should be able to setNativeProps on native refs', () => {
it('setNativeProps on native refs should no-op', () => {
const View = createReactNativeComponentClass('RCTView', () => ({
validAttributes: {foo: true},
uiViewClassName: 'RCTView',
Expand All @@ -252,13 +244,12 @@ describe('ReactFabric', () => {
);

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'},
);
expect(() => {
ReactFabric.setNativeProps(viewRef, {foo: 'baz'});
}).toWarnDev([SET_NATIVE_PROPS_NOT_SUPPORTED_MESSAGE], {
withoutStack: true,
});
expect(UIManager.updateView).not.toBeCalled();
});

it('should warn and no-op if calling setNativeProps on non native refs', () => {
Expand Down Expand Up @@ -303,14 +294,9 @@ describe('ReactFabric', () => {
expect(UIManager.updateView).not.toBeCalled();
expect(() => {
ReactFabric.setNativeProps(viewRef, {foo: 'baz'});
}).toWarnDev(
[
"Warning: setNativeProps was called with a ref that isn't a " +
'native component. Use React.forwardRef to get access ' +
'to the underlying native component',
],
{withoutStack: true},
);
}).toWarnDev([SET_NATIVE_PROPS_NOT_SUPPORTED_MESSAGE], {
withoutStack: true,
});

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