Check for createElement being called from React #1011
Conversation
Fixes jsx-eslint#996. This will prevent document.createElement triggering a false positive for react/display-name.
| @@ -258,6 +258,8 @@ function componentRule(rule, context) { | |||
| var returnsReactCreateElement = | |||
| node[property] && | |||
| node[property].callee && | |||
| node[property].callee.object && | |||
| node[property].callee.object.name === 'React' && | |||
There was a problem hiding this comment.
won't this prevent import { createElement } from 'react'; from being detected?
There was a problem hiding this comment.
You're right. I found a way to check for this by using the ModuleScope of the Variable when importing createElement like that. However, it involves using Map, and it doesn't seem like I can get away with that with the project setup.
There was a problem hiding this comment.
I'll just temporarily add in a disable comment so I can feel good about pushing it up. I'll remove it after receiving feedback.
Due to the previous change of checking for `createElement` being called on React, this left out the use case of a destructured import of `createElement`.
| if (variable !== null) { | ||
| var map = variable.scope.set; | ||
| // eslint-disable-next-line no-undef | ||
| if (map instanceof Map && map.has('React')) { |
There was a problem hiding this comment.
There's no need to use Map if the keys are strings; you can just use a normal object.
There was a problem hiding this comment.
I might be misunderstanding, but that is what it returns when I pull it out of the ModuleScope. Wouldn't I need to convert it to an object to use it like one?
There was a problem hiding this comment.
oops, sorry, if that's what it's doing then this is correct :-)
There was a problem hiding this comment.
altho i'm confused about the instanceof Map check - when will that be false? eslint requires node 4 or later.
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
return variables.find(function (variable) { return variable.name === name; });There was a problem hiding this comment.
I prefer this, so I'll go with it. I was mimicking the findVariable function already there.
There was a problem hiding this comment.
might as well update both then ;-)
| @@ -548,5 +561,16 @@ ruleTester.run('display-name', rule, { | |||
| errors: [{ | |||
| message: 'Component definition is missing display name' | |||
| }] | |||
| }, { | |||
| code: [ | |||
| 'import React, { createElement } from "react";', | |||
There was a problem hiding this comment.
Let's also add cases for var { createElement } = React; and var createElement = React.createElement;.
Probably too difficult to cover var c = React.createElement; tho.
| * @returns {Object} Variable if the variable was found, null if not. | ||
| */ | ||
| function getVariable(variables, name) { | ||
| return variables.find(function (variable) { |
There was a problem hiding this comment.
No, we absolutely must fix this first, since otherwise it's a semver-major change, and we need to publish master before a semver-major line.
There was a problem hiding this comment.
Sounds good. I'll put up a PR shortly.
There was a problem hiding this comment.
No worries; I pushed up e332b08 to fix it
There was a problem hiding this comment.
Oh nevermind looks like you fixed it in e332b08
Fixes #996. This will prevent document.createElement triggering a
false positive for react/display-name.