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.

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

Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps #14912

Merged
merged 5 commits into from
Feb 25, 2019
Merged

Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps #14912

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d951967
Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps
elicwhite Feb 21, 2019
e8d7049
Using a feature flag for the setNativeProps warning
elicwhite Feb 24, 2019
5382f82
Removing extra line breaks
elicwhite Feb 24, 2019
762c59a
Set the FB native feature flag to true
elicwhite Feb 25, 2019
f863070
Prettier
elicwhite Feb 25, 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
14 changes: 14 additions & 0 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 @@ -27,6 +27,9 @@ import {
warnForStyleProps,
} from './NativeMethodsMixinUtils';

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

export default function(
findNodeHandle: any => ?number,
findHostInstance: any => any,
Expand Down Expand Up @@ -121,6 +124,17 @@ 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 Down
11 changes: 11 additions & 0 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 @@ -30,6 +30,8 @@ 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 @@ -140,6 +142,15 @@ 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);
}

Expand Down
15 changes: 15 additions & 0 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 @@ -24,6 +24,9 @@ import UIManager from 'UIManager';
import {create} from './ReactNativeAttributePayload';
import {mountSafeCallback_NOT_REALLY_SAFE} from './NativeMethodsMixinUtils';

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

export default function(
findNodeHandle: any => ?number,
findHostInstance: any => any,
Expand Down Expand Up @@ -132,6 +135,18 @@ 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 Down
12 changes: 12 additions & 0 deletions packages/react-native-renderer/src/ReactNativeFiberHostComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ import {
warnForStyleProps,
} from './NativeMethodsMixinUtils';

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

/**
* This component defines the same methods as NativeMethodsMixin but without the
* findNodeHandle wrapper. This wrapper is unnecessary for HostComponent views
Expand Down Expand Up @@ -81,6 +84,15 @@ class ReactNativeFiberHostComponent {

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);
}

Expand Down
22 changes: 20 additions & 2 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 @@ -12,13 +12,20 @@

let React;
let ReactFabric;
let ReactFeatureFlags;
let createReactClass;
let createReactNativeComponentClass;
let UIManager;
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";

jest.mock('shared/ReactFeatureFlags', () =>
require('shared/forks/ReactFeatureFlags.native-oss'),
);
Expand All @@ -29,6 +36,8 @@ describe('ReactFabric', () => {

React = require('react');
StrictMode = React.StrictMode;
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.warnAboutDeprecatedSetNativeProps = true;
ReactFabric = require('react-native-renderer/fabric');
FabricUIManager = require('FabricUIManager');
UIManager = require('UIManager');
Expand Down Expand Up @@ -201,10 +210,19 @@ describe('ReactFabric', () => {
);
expect(UIManager.updateView).not.toBeCalled();

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

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

viewRef.setNativeProps({foo: 'baz'});
expect(() => {
viewRef.setNativeProps({foo: 'baz'});
}).toWarnDev([SET_NATIVE_PROPS_DEPRECATION_MESSAGE], {
withoutStack: true,
});
expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
expect.any(Number),
Expand Down
26 changes: 23 additions & 3 deletions packages/react-native-renderer/src/__tests__/ReactNativeMount-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,28 @@
'use strict';

let React;
let ReactFeatureFlags;
let StrictMode;
let ReactNative;
let createReactClass;
let createReactNativeComponentClass;
let UIManager;
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";

describe('ReactNative', () => {
beforeEach(() => {
jest.resetModules();

React = require('react');
StrictMode = React.StrictMode;
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.warnAboutDeprecatedSetNativeProps = true;
ReactNative = require('react-native-renderer');
UIManager = require('UIManager');
createReactClass = require('create-react-class/factory')(
Expand Down Expand Up @@ -98,7 +107,7 @@ describe('ReactNative', () => {
expect(UIManager.updateView).toHaveBeenCalledTimes(4);
});

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 @@ -132,10 +141,19 @@ describe('ReactNative', () => {
);
expect(UIManager.updateView).not.toBeCalled();

viewRef.setNativeProps({});
expect(() => {
viewRef.setNativeProps({});
}).toWarnDev([SET_NATIVE_PROPS_DEPRECATION_MESSAGE], {
withoutStack: true,
});
expect(UIManager.updateView).not.toBeCalled();

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

expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
expect.any(Number),
Expand Down Expand Up @@ -164,7 +182,9 @@ describe('ReactNative', () => {
11,
);

ReactNative.setNativeProps(viewRef, {});
expect(UIManager.updateView).not.toBeCalled();

ReactNative.setNativeProps(viewRef, {foo: 'baz'});
expect(UIManager.updateView).toHaveBeenCalledTimes(1);
expect(UIManager.updateView).toHaveBeenCalledWith(
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/ReactFeatureFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,7 @@ export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;

export const warnAboutShorthandPropertyCollision = false;

// See https://github.com/react-native-community/discussions-and-proposals/issues/72 for more information
// This is a flag so we can fix warnings in RN core before turning it on
export const warnAboutDeprecatedSetNativeProps = false;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const debugRenderPhaseSideEffectsForStrictMode = true;
export const disableInputAttributeSyncing = false;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const warnAboutDeprecatedLifecycles = true;
export const warnAboutDeprecatedSetNativeProps = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. Is this the canonical one?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by canonical? It seems like to get Flow to pass these consts have to be defined in every file

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, nvm. I understand. I need to re-export the one from FeatureFlags. I'm testing this change more closely to ensure I can actually flip this on in FBSource.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should actually remove it from the dynamic one if possible. Whenever possible we should always use static flags and drive to remove the dynamic ones.


// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;
export const enableSchedulerDebugging = false;
export const warnAboutDeprecatedSetNativeProps = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.persistent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;
export const enableSchedulerDebugging = false;
export const warnAboutDeprecatedSetNativeProps = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;
export const enableSchedulerDebugging = false;
export const warnAboutDeprecatedSetNativeProps = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const enableSchedulerTracing = false;
export const enableSuspenseServerRenderer = false;
export const enableStableConcurrentModeAPIs = false;
export const enableSchedulerDebugging = false;
export const warnAboutDeprecatedSetNativeProps = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const {
warnAboutDeprecatedLifecycles,
disableInputAttributeSyncing,
warnAboutShorthandPropertyCollision,
warnAboutDeprecatedSetNativeProps,
} = require('ReactFeatureFlags');

// In www, we have experimental support for gathering data
Expand Down