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 for functional refs on stateless functional components #7272

Closed
Closed
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
14 changes: 13 additions & 1 deletion src/renderers/shared/stack/reconciler/ReactRef.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
'use strict';

var ReactOwner = require('ReactOwner');
var warning = require('warning');

import type { ReactInstance } from 'ReactInstanceType';
import type { ReactElement } from 'ReactElementType';
Expand All @@ -21,7 +22,18 @@ var ReactRef = {};

function attachRef(ref, component, owner) {
if (typeof ref === 'function') {
ref(component.getPublicInstance());
var instance = component.getPublicInstance();
if (__DEV__) {
var componentName = component && component.getName ?
Copy link
Contributor

Choose a reason for hiding this comment

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

var componentName = component.getName() || 'a component';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

component.getName is not always going to be defined here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, I guess you can remove component && ... though

component.getName() : 'a component';
warning(instance != null,
'Stateless function components cannot be given refs ' +
'(See %s%s).',
componentName,
owner ? ' created by ' + owner.getName() : ''
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should follow the normal pattern check the render method of ...
Something like Stateless functional component %s cannot have refs attached. Check the render method of %s. I also think that the other message (when using a string ref) should follow this pattern too.

);
}
ref(instance);
} else {
// Legacy ref
ReactOwner.addComponentAsRefTo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,25 @@ describe('ReactStatelessComponent', () => {
);
});

it('should warn when given a ref', () => {
it('should warn for functional refs in pure functions', function() {
spyOn(console, 'error');

var Parent = React.createClass({
displayName: 'Parent',
render: function() {
return <StatelessComponent name="A" ref={() => {}}/>;
},
});

ReactTestUtils.renderIntoDocument(<Parent />);
expect(console.error.calls.count()).toBe(1);
expect(console.error.calls.argsFor(0)[0]).toContain(
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to use toEqual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use toContain so I don't have to include the "Warning: " prefix in the check.

'Stateless function components cannot be given refs ' +
'(See StatelessComponent created by Parent).',
);
});

it('should warn when given a ref', function() {
spyOn(console, 'error');

class Parent extends React.Component {
Expand Down