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

Usage of .name|.displayName to identify stateless functional components #6161

Closed
wants to merge 4 commits into from
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
2 changes: 1 addition & 1 deletion docs/docs/05-reusable-components.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ ReactDOM.render(<HelloMessage name="Sebastian" />, mountNode);
```

This simplified component API is intended for components that are pure functions of their props. These components must not retain internal state, do not have backing instances, and do not have the component lifecycle methods. They are pure functional transforms of their input, with zero boilerplate.
However, you may still specify `.propTypes` and `.defaultProps` by setting them as properties on the function, just as you would set them on an ES6 class.
However, you may still specify `.propTypes`, `.defaultProps`, and `.displayName` by setting them as properties on the function, just as you would set them on an ES6 class. `.displayName` or `.name` may be used as the name of the component within debug messages if it's a non-empty string. `.displayName` would take precedence over `.name` when calculating the component name.

> NOTE:
>
Expand Down
3 changes: 2 additions & 1 deletion src/isomorphic/classic/element/ReactElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var ReactCurrentOwner = require('ReactCurrentOwner');
var assign = require('Object.assign');
var warning = require('warning');
var canDefineProperty = require('canDefineProperty');
var getComponentName = require('getComponentName');

// The Symbol used to tag the ReactElement type. If there is no native Symbol
// nor polyfill, then a plain number is used for performance.
Expand Down Expand Up @@ -184,7 +185,7 @@ ReactElement.createElement = function(type, config, children) {
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (https://fb.me/react-special-props)',
'displayName' in type ? type.displayName: 'Element'
getComponentName(type) || 'Element'
);
}
return undefined;
Expand Down
120 changes: 95 additions & 25 deletions src/isomorphic/classic/element/__tests__/ReactElement-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,35 +57,105 @@ describe('ReactElement', function() {
expect(element.props).toEqual(expectation);
});

it('should warn when `key` is being accessed', function() {
/**
* Check the warning about accessing props.key for correct component name.
*
* @param {object} testOpts Options for the test.
* testOpts.Component {ReactClass|function} ReactComponent class or
* stateless function.
*
* testOpts.expectedName {string} The name that the component is expected to
* be identified by in the warning message.
*
* @return {void}
*/
function testKeyAccess(testOpts) {
/**
* Generate the expected warning message for accessing props.key.
*
* @param {object} opts Options.
* opts.name {string} Component name.
*
* @return {string}
*/
function getWarning(opts) {
return `
${opts.name}: ${'`key`'} is not a prop. Trying to access it will result
in ${'`undefined`'} being returned. If you need to access the same
value within the child component, you should pass it as a different
prop. (https://fb.me/react-special-props)
`.replace(/\n */g, ' ').trim();
}

spyOn(console, 'error');
var {Component, expectedName} = testOpts;
var container = document.createElement('div');
var Child = React.createClass({
render: function() {
return <div> {this.props.key} </div>;
},
});
var Parent = React.createClass({
render: function() {
return (
<div>
<Child key="0" />
<Child key="1" />
<Child key="2" />
</div>
);
},
});

function render(props) {
return <div> {props.key} </div>;
}

expect(console.error.calls.length).toBe(0);
ReactDOM.render(<Parent />, container);
ReactDOM.render(<Component key="0" render={render} />, container);
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain(
'Child: `key` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (https://fb.me/react-special-props)'
);
});
expect(console.error.argsForCall[0][0]).toContain(getWarning({
name: expectedName,
}));
}

it(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having many tests like this seems unnecessary to me. If we extract getComponentName into a function, we can get away with one test here and a bunch of tests for specific cases in getComponentName-test. The indirection with testKeyAccess seems over-abstracted and unnecessary to me. Can we make this file simple again?

'should warn and identify Component class by `.displayName` when `props.key` is being accessed',

function() {
var Something = React.createClass({
render: function() {
return this.props.render(this.props);
},
});

// Test that warning message about accessing props.key identifies
// Component as expectedName.
return testKeyAccess({
Component: Something,
expectedName: Something.displayName,
});
}
);

it(
'should warn and identify stateless function by `.name` when `props.key` is being accessed',

function() {
function Something(props) {
return props.render(props);
}

// Test that warning message about accessing props.key identifies
// Component as expectedName.
return testKeyAccess({
Component: Something,
expectedName: Something.name,
});
}
);

it(
'should warn and identify stateless function by `.displayName` when `props.key` is being accessed',

function() {
function Something(props) {
return props.render(props);
}
Something.displayName = Something.name + 'with displayName';

// Test that warning message about accessing props.key identifies
// Component as expectedName.
return testKeyAccess({
Component: Something,
expectedName: Something.displayName,
});
}
);

it('should warn when `ref` is being accessed', function() {
spyOn(console, 'error');
Expand Down
28 changes: 28 additions & 0 deletions src/shared/utils/__tests__/getComponentName-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

var getComponentName = require('getComponentName');
var React = require('React');

describe('getComponentName', function() {
it('should prefer `displayName`', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let’s structure it like this:

  • describe('getComponentName')
    • describe('stateless functional component')
      • it('should prefer displayName')
      • it('should use name when there is no displayName')
      • it('should fall back to ReactComponent')
    • describe('ES class')
      • it('should prefer displayName')
      • it('should use name when there is no displayName')
      • it('should fall back to ReactComponent')
    • describe('createClass')
      • it('should prefer displayName')
      • it('should fall back to ReactComponent')

or something similar.

var displayName = 'Else';

var Something = function Something() {};
Something.displayName = displayName;

expect(getComponentName(Something)).toBe(displayName);

Something = React.createClass({
render: function() {},
displayName: displayName,
});

expect(getComponentName(Something)).toBe(displayName);
});

it('should fall back on `name`', function() {
function Something() {}

expect(getComponentName(Something)).toBe(Something.name);
});
});
27 changes: 27 additions & 0 deletions src/shared/utils/getComponentName.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright 2013-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule getComponentName
*/

'use strict';

/**
* Returns the component name, if any.
*
* This is to provide consistent logic for choosing the name: prefer
* `displayName` and fall back on `name`.
*
* @param {ReactComponent} component
* @return {?string} Name, if any.
*/
function getComponentName(component) {
return component && (component.displayName || component.name);
}

module.exports = getComponentName;