Add displayName to more types of React components. #5679
Conversation
This adds support for the two other kinds of React component definitions not previously covered by the existing displayName plugin: - ES6-classes style components - Stateless components that return JSX
|
@roncohen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @existentialism and @loganfsmyth to be potential reviewers. |
Codecov Report
@@ Coverage Diff @@
## 7.0 #5679 +/- ##
======================================
Coverage ? 84.62%
======================================
Files ? 283
Lines ? 9802
Branches ? 2758
======================================
Hits ? 8295
Misses ? 988
Partials ? 519
Continue to review full report at Codecov.
|
| const name = pathMod.basename(state.file.opts.filename, extension); | ||
|
|
||
| const id = path.scope.generateUidIdentifier("uid"); | ||
| path.node.id = id; |
There was a problem hiding this comment.
This actually transforms the declaration to export default function _uid () {}, unlike what the comment says. It works the same, though.
However, given how this transform, when enabled, would run before any module or function naming transform, it would change the actual function name to something other than "default" :/
Maybe wrapping it like this could work:
var _uid = /*#__PURE__*/(function () {
var func = { 'default': function () { ... } }['default']
func.displayName = 'default' // or whatever you inferred
return func
})()
export { _uid as default }You can use babel-template to generate the replacement AST.
Note, export { _uid as default } and not export default _uid.
| // to `var _uid1 = function () { .. }; export default __uid;` | ||
| // then add displayName to _uid1 | ||
| const extension = pathMod.extname(state.file.opts.filename); | ||
| const name = pathMod.basename(state.file.opts.filename, extension); |
There was a problem hiding this comment.
This could lead to an awful amount of components with index as the displayName 😄
Though if you didn't use any heuristics at all they'd just be named the equally unhelpful default.
There was a problem hiding this comment.
What do you think about using the folder name if the filename is index?
|
|
||
| function doesReturnJSX (body) { | ||
| if (!body) return false; | ||
| if (body.type === "JSXElement") { |
There was a problem hiding this comment.
Since React 15 null is also a valid return type, and with React 16 so will an array of JSXElement; though missing these for now is probably fine as it's just a heuristic.
| const members = node.node.body.body; | ||
| for (let i = 0; i < members.length; i++) { | ||
| if (members[i].type == "ClassMethod" && members[i].key.name == "render") { | ||
| return true; |
There was a problem hiding this comment.
Checking if the class has a non-null extends clause could help reduce false positives, as well as checking if render doesReturnJSX.
| }, | ||
| ClassDeclaration: function (path) { | ||
| if (classHasRenderMethod(path)) { | ||
| setDisplayNameAfter(path, path.node.id); |
There was a problem hiding this comment.
Should be able to skip this if the class has a static displayName = (or static get displayName () {}).
Supporting static displayName = would add a soft dependency on running before the class properties transform, though; but that should be the case in most setups.
|
@roncohen Do you still intend to come back to this? I might be able to take this over if you don't have time. |
|
@smably still intend to take a look at this, just been super busy. Hopefully in the weekend! |
|
Is this PR stalled now? |
|
Looks like it, someone can do a new pr + rebase it? |
|
“classes are considered to be react components if they have a render method” will also target Backbone views. |
|
@ljharb Not really; you can't use |
|
@mAAdhaTTah it's bitten eslint-plugin-react before ¯\_(ツ)_/¯ component detection is hard. |
|
@mAAdhaTTah I'll chip in and say that at my last job, I converted our entire Backbone codebase to ES6 classes, so this would definitely have affected that codebase. Whether or not the new property would actually hurt anything, or if users are enabling |
|
Fair enough, I stand corrected. :) |
|
Sooo when is this getting merged in? 👀 |
This adds support for the two other kinds of React component definitions not previously covered by the existing displayName plugin:
See #4663 for a brief discussion.
Heuristics
ES6 classes are considered to be React components if they have a
rendermethod. Functions are considered components if they return JSX on the last line.In our testing, these are quite accurate heuristics. Besides, the cost of erroneously determining a function/class to be a component and then proceeding to set
displayNameon a non-component should not be a big problem.There are some nuances involved to make it work. Please see the tests for details.