Skip to content
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
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1538,6 +1538,7 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
* should throw on string refs in pure functions
* should warn when given a string ref
* should warn when given a function ref
* deduplicates ref warnings based on element or owner
* should provide a null ref
* should use correct name in key warning
* should support default props and prop types
Expand Down
24 changes: 17 additions & 7 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ var warning = require('warning');

if (__DEV__) {
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');

var warnedAboutStatelessRefs = {};
}

module.exports = function<T, P, I, TI, C, CX, CI>(
Expand Down Expand Up @@ -464,13 +466,21 @@ module.exports = function<T, P, I, TI, C, CX, CI>(
info += ' Check the render method of `' + ownerName + '`.';
}

warning(
false,
'Stateless function components cannot be given refs. ' +
'Attempts to access this ref will fail.%s%s',
info,
ReactDebugCurrentFiber.getCurrentFiberStackAddendum()
);
let warningKey = ownerName || workInProgress._debugID || '';
const debugSource = workInProgress._debugSource;
Copy link
Contributor

Choose a reason for hiding this comment

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

_debugSource is added via a babel plugin, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is added here to the Fiber, and it is on the element from this Babel plugin (enabled by default in CRA).

Copy link
Contributor

Choose a reason for hiding this comment

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

not directly related to this PR, but are there any concerns that dev behavior may change slightly based on babel-plugins? Especially for compile-to-js implementations?

Perhaps there should be documentation added to https://facebook.github.io/react/contributing/implementation-notes.html for _debugSource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would be nice to add, want to send a PR?

if (debugSource) {
warningKey = debugSource.fileName + ':' + debugSource.lineNumber;
}
if (!warnedAboutStatelessRefs[warningKey]) {
warnedAboutStatelessRefs[warningKey] = true;
warning(
false,
'Stateless function components cannot be given refs. ' +
'Attempts to access this ref will fail.%s%s',
info,
ReactDebugCurrentFiber.getCurrentFiberStackAddendum()
);
}
}
}
reconcileChildren(current, workInProgress, value);
Expand Down
108 changes: 93 additions & 15 deletions src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe('ReactStatelessComponent', () => {
}

beforeEach(() => {
jest.resetModuleRegistry();
React = require('React');
ReactDOM = require('ReactDOM');
ReactTestUtils = require('ReactTestUtils');
Expand Down Expand Up @@ -156,54 +157,131 @@ describe('ReactStatelessComponent', () => {
return <div>{props.children}</div>;
}

class Parent extends React.Component {
class ParentUsingStringRef extends React.Component {
render() {
return <Indirection><StatelessComponent name="A" ref="stateless"/></Indirection>;
return (
<Indirection>
<StatelessComponent name="A" ref="stateless" />
</Indirection>
);
}
}

ReactTestUtils.renderIntoDocument(<Parent/>);

ReactTestUtils.renderIntoDocument(<ParentUsingStringRef />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Stateless function components cannot be given refs. ' +
'Attempts to access this ref will fail. Check the render method ' +
'of `Parent`.\n' +
'of `ParentUsingStringRef`.\n' +
' in StatelessComponent (at **)\n' +
' in div (at **)\n' +
' in Indirection (at **)\n' +
' in Parent (at **)'
' in ParentUsingStringRef (at **)'
);

ReactTestUtils.renderIntoDocument(<ParentUsingStringRef />);
expectDev(console.error.calls.count()).toBe(1);
});

it('should warn when given a function ref', () => {
spyOn(console, 'error');
var ref = jasmine.createSpy().and.callFake((arg) => {
expect(arg).toBe(null);
});

function Indirection(props) {
return <div>{props.children}</div>;
}

class Parent extends React.Component {
class ParentUsingFunctionRef extends React.Component {
render() {
return <Indirection><StatelessComponent name="A" ref={ref} /></Indirection>;
return (
<Indirection>
<StatelessComponent name="A" ref={(arg) => {
expect(arg).toBe(null);
}} />
</Indirection>
);
}
}

ReactTestUtils.renderIntoDocument(<Parent/>);

ReactTestUtils.renderIntoDocument(<ParentUsingFunctionRef />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: Stateless function components cannot be given refs. ' +
'Attempts to access this ref will fail. Check the render method ' +
'of `Parent`.\n' +
'of `ParentUsingFunctionRef`.\n' +
' in StatelessComponent (at **)\n' +
' in div (at **)\n' +
' in Indirection (at **)\n' +
' in Parent (at **)'
' in ParentUsingFunctionRef (at **)'
);

ReactTestUtils.renderIntoDocument(<ParentUsingFunctionRef />);
expectDev(console.error.calls.count()).toBe(1);
});

it('deduplicates ref warnings based on element or owner', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

+1. this test looks very comprehensive.

spyOn(console, 'error');

// Prevent the Babel transform adding a displayName.
var createClassWithoutDisplayName = React.createClass;
Copy link
Contributor

Choose a reason for hiding this comment

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

if the babel plugin is updated to follow React.createClass aliases I assume this test would break?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea. But I bet pretty hard it won't happen. :-)


// When owner uses JSX, we can use exact line location to dedupe warnings
var AnonymousParentUsingJSX = createClassWithoutDisplayName({
render() {
return <StatelessComponent name="A" ref={() => {}} />;
},
});
const instance1 = ReactTestUtils.renderIntoDocument(<AnonymousParentUsingJSX />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Stateless function components cannot be given refs.'
);
// Should be deduped (offending element is on the same line):
instance1.forceUpdate();
// Should also be deduped (offending element is on the same line):
ReactTestUtils.renderIntoDocument(<AnonymousParentUsingJSX />);
expectDev(console.error.calls.count()).toBe(1);
console.error.calls.reset();

// When owner doesn't use JSX, and is anonymous, we warn once per internal instance.
var AnonymousParentNotUsingJSX = createClassWithoutDisplayName({
render() {
return React.createElement(StatelessComponent, {name: 'A', 'ref': () => {}});
},
});
const instance2 = ReactTestUtils.renderIntoDocument(<AnonymousParentNotUsingJSX />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Stateless function components cannot be given refs.'
);
// Should be deduped (same internal instance):
instance2.forceUpdate();
expectDev(console.error.calls.count()).toBe(1);
// Could not be deduped (different internal instance):
ReactTestUtils.renderIntoDocument(<AnonymousParentNotUsingJSX />);
expectDev(console.error.calls.count()).toBe(2);
expectDev(console.error.calls.argsFor(1)[0]).toContain(
'Warning: Stateless function components cannot be given refs.'
);
console.error.calls.reset();

// When owner doesn't use JSX, but is named, we warn once per owner name
class NamedParentNotUsingJSX extends React.Component {
render() {
return React.createElement(StatelessComponent, {name: 'A', 'ref': () => {}});
}
}
const instance3 = ReactTestUtils.renderIntoDocument(<NamedParentNotUsingJSX />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Warning: Stateless function components cannot be given refs.'
);
// Should be deduped (same owner name):
instance3.forceUpdate();
expectDev(console.error.calls.count()).toBe(1);
// Should also be deduped (same owner name):
ReactTestUtils.renderIntoDocument(<NamedParentNotUsingJSX />);
expectDev(console.error.calls.count()).toBe(1);
console.error.calls.reset();
});

it('should provide a null ref', () => {
Expand Down
40 changes: 26 additions & 14 deletions src/renderers/shared/stack/reconciler/ReactRef.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,40 @@ if (__DEV__) {
var ReactCompositeComponentTypes = require('ReactCompositeComponentTypes');
var ReactComponentTreeHook = require('ReactComponentTreeHook');
var warning = require('warning');

var warnedAboutStatelessRefs = {};
}

function attachRef(ref, component, owner) {
if (__DEV__) {
let info = '';
if (owner) {
if (component._compositeType === ReactCompositeComponentTypes.StatelessFunctional) {
let info = '';
let ownerName;
if (typeof owner.getName === 'function') {
ownerName = owner.getName();
if (owner) {
if (typeof owner.getName === 'function') {
ownerName = owner.getName();
}
if (ownerName) {
info += ' Check the render method of `' + ownerName + '`.';
}
}

let warningKey = ownerName || component._debugID;
let element = component._currentElement;
if (element && element._source) {
warningKey = element._source.fileName + ':' + element._source.lineNumber;
}
if (ownerName) {
info += ' Check the render method of `' + ownerName + '`.';
if (!warnedAboutStatelessRefs[warningKey]) {
warnedAboutStatelessRefs[warningKey] = true;
warning(
false,
'Stateless function components cannot be given refs. ' +
'Attempts to access this ref will fail.%s%s',
info,
ReactComponentTreeHook.getStackAddendumByID(component._debugID)
);
}
}

warning(
component._compositeType !== ReactCompositeComponentTypes.StatelessFunctional,
'Stateless function components cannot be given refs. ' +
'Attempts to access this ref will fail.%s%s',
info,
ReactComponentTreeHook.getStackAddendumByID(component._debugID)
);
}

if (typeof ref === 'function') {
Expand Down