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

Warn about string refs within strict mode trees #12161

Merged
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {ReactPortal} from 'shared/ReactTypes';
import type {Fiber} from 'react-reconciler/src/ReactFiber';
import type {ExpirationTime} from 'react-reconciler/src/ReactFiberExpirationTime';

import getComponentName from 'shared/getComponentName';
import {Placement, Deletion} from 'shared/ReactTypeOfSideEffect';
import {
getIteratorFn,
Expand All @@ -26,6 +27,7 @@ import {
HostPortal,
Fragment,
} from 'shared/ReactTypeOfWork';
import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook';
import emptyObject from 'fbjs/lib/emptyObject';
import invariant from 'fbjs/lib/invariant';
import warning from 'fbjs/lib/warning';
Expand All @@ -38,16 +40,20 @@ import {
createFiberFromPortal,
} from './ReactFiber';
import ReactDebugCurrentFiber from './ReactDebugCurrentFiber';
import {StrictMode} from './ReactTypeOfMode';

const {getCurrentFiberStackAddendum} = ReactDebugCurrentFiber;

let didWarnAboutMaps;
let didWarnAboutStringRefInStrictMode;
let ownerHasKeyUseWarning;
let ownerHasFunctionTypeWarning;
let warnForMissingKey = (child: mixed) => {};

if (__DEV__) {
didWarnAboutMaps = false;
didWarnAboutStringRefInStrictMode = {};

/**
* Warn if there's no key explicitly set on dynamic arrays of children or
* object keys are not valid. This allows us to keep track of children between
Expand Down Expand Up @@ -92,9 +98,32 @@ if (__DEV__) {

const isArray = Array.isArray;

function coerceRef(current: Fiber | null, element: ReactElement) {
function coerceRef(
returnFiber: Fiber,
current: Fiber | null,
element: ReactElement,
) {
let mixedRef = element.ref;
if (mixedRef !== null && typeof mixedRef !== 'function') {
if (__DEV__) {
if (returnFiber.mode & StrictMode) {
const componentName = getComponentName(returnFiber) || 'Component';
if (!didWarnAboutStringRefInStrictMode[componentName]) {
warning(
false,
'A string ref has been found within a strict mode tree. ' +
'String refs are a source of potential bugs and should be avoided. ' +
'We recommend using a ref callback instead.' +
'\n%s' +
'\n\nLearn more about using refs safely here:' +
'\nhttps://fb.me/react-strict-mode-string-ref',
getStackAddendumByWorkInProgressFiber(returnFiber),
);
didWarnAboutStringRefInStrictMode[componentName] = true;
}
}
}

if (element._owner) {
const owner: ?Fiber = (element._owner: any);
let inst;
Expand Down Expand Up @@ -340,7 +369,7 @@ function ChildReconciler(shouldTrackSideEffects) {
if (current !== null && current.type === element.type) {
// Move based on index
const existing = useFiber(current, element.props, expirationTime);
existing.ref = coerceRef(current, element);
existing.ref = coerceRef(returnFiber, current, element);
existing.return = returnFiber;
if (__DEV__) {
existing._debugSource = element._source;
Expand All @@ -354,7 +383,7 @@ function ChildReconciler(shouldTrackSideEffects) {
returnFiber.mode,
expirationTime,
);
created.ref = coerceRef(current, element);
created.ref = coerceRef(returnFiber, current, element);
created.return = returnFiber;
return created;
}
Expand Down Expand Up @@ -439,7 +468,7 @@ function ChildReconciler(shouldTrackSideEffects) {
returnFiber.mode,
expirationTime,
);
created.ref = coerceRef(null, newChild);
created.ref = coerceRef(returnFiber, null, newChild);
created.return = returnFiber;
return created;
}
Expand Down Expand Up @@ -1077,7 +1106,7 @@ function ChildReconciler(shouldTrackSideEffects) {
: element.props,
expirationTime,
);
existing.ref = coerceRef(child, element);
existing.ref = coerceRef(returnFiber, child, element);
existing.return = returnFiber;
if (__DEV__) {
existing._debugSource = element._source;
Expand Down Expand Up @@ -1109,7 +1138,7 @@ function ChildReconciler(shouldTrackSideEffects) {
returnFiber.mode,
expirationTime,
);
created.ref = coerceRef(currentFirstChild, element);
created.ref = coerceRef(returnFiber, currentFirstChild, element);
created.return = returnFiber;
return created;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,9 @@ describe('ReactIncrementalSideEffects', () => {
}

ReactNoop.render(<Foo />);
ReactNoop.flush();
expect(ReactNoop.flush).toWarnDev(
'Warning: A string ref has been found within a strict mode tree.',
);

expect(fooInstance.refs.bar.test).toEqual('test');
});
Expand Down
85 changes: 85 additions & 0 deletions packages/react/src/__tests__/ReactStrictMode-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -745,4 +745,89 @@ describe('ReactStrictMode', () => {
expect(rendered.toJSON()).toBe('count:2');
});
});

describe('string refs', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactTestRenderer = require('react-test-renderer');
});

it('should warn within a strict tree', () => {
const {StrictMode} = React;

class OuterComponent extends React.Component {
render() {
return (
<StrictMode>
<InnerComponent ref="somestring" />
</StrictMode>
);
}
}

class InnerComponent extends React.Component {
render() {
return null;
}
}

let renderer;
expect(() => {
renderer = ReactTestRenderer.create(<OuterComponent />);
}).toWarnDev(
'Warning: A string ref has been found within a strict mode tree. ' +
'String refs are a source of potential bugs and should be avoided. ' +
'We recommend using a ref callback instead.\n\n' +
' in OuterComponent (at **)\n\n' +
'Learn more about using refs safely here:\n' +
'https://fb.me/react-strict-mode-string-ref',
);

// Dedup
renderer.update(<OuterComponent />);
});

it('should warn within a strict tree', () => {
const {StrictMode} = React;

class OuterComponent extends React.Component {
render() {
return (
<StrictMode>
<InnerComponent />
</StrictMode>
);
}
}

class InnerComponent extends React.Component {
render() {
return <MiddleComponent ref="somestring" />;
}
}

class MiddleComponent extends React.Component {
render() {
return null;
}
}

let renderer;
expect(() => {
renderer = ReactTestRenderer.create(<OuterComponent />);
}).toWarnDev(
'Warning: A string ref has been found within a strict mode tree. ' +
'String refs are a source of potential bugs and should be avoided. ' +
'We recommend using a ref callback instead.\n\n' +
' in InnerComponent (at **)\n' +
' in OuterComponent (at **)\n\n' +
'Learn more about using refs safely here:\n' +
'https://fb.me/react-strict-mode-string-ref',
);

// Dedup
renderer.update(<OuterComponent />);
});
});
});