-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Check for createElement being called from React #1011
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,16 +11,21 @@ | |
* @returns {Boolean} True if the variable was found, false if not. | ||
*/ | ||
function findVariable(variables, name) { | ||
var i; | ||
var len; | ||
|
||
for (i = 0, len = variables.length; i < len; i++) { | ||
if (variables[i].name === name) { | ||
return true; | ||
} | ||
} | ||
return variables.some(function (variable) { | ||
return variable.name === name; | ||
}); | ||
} | ||
|
||
return false; | ||
/** | ||
* Find and return a particular variable in a list | ||
* @param {Array} variables The variables list. | ||
* @param {Array} name The name of the variable to search. | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. I'll put up a PR shortly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries; I pushed up e332b08 to fix it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nevermind looks like you fixed it in e332b08 |
||
return variable.name === name; | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -52,5 +57,6 @@ function variablesInScope(context) { | |
|
||
module.exports = { | ||
findVariable: findVariable, | ||
getVariable: getVariable, | ||
variablesInScope: variablesInScope | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,6 +378,19 @@ ruleTester.run('display-name', rule, { | |
')' | ||
].join('\n'), | ||
parser: 'babel-eslint' | ||
}, { | ||
code: [ | ||
'module.exports = {', | ||
' createElement: tagName => document.createElement(tagName)', | ||
'};' | ||
].join('\n'), | ||
parser: 'babel-eslint' | ||
}, { | ||
code: [ | ||
'const { createElement } = document;', | ||
'createElement("a");' | ||
].join('\n'), | ||
parser: 'babel-eslint' | ||
}], | ||
|
||
invalid: [{ | ||
|
@@ -548,5 +561,40 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also add cases for Probably too difficult to cover |
||
'export default (props) => {', | ||
' return createElement("div", {}, "hello");', | ||
'};' | ||
].join('\n'), | ||
parser: 'babel-eslint', | ||
errors: [{ | ||
message: 'Component definition is missing display name' | ||
}] | ||
}, { | ||
code: [ | ||
'import React from "react";', | ||
'const { createElement } = React;', | ||
'export default (props) => {', | ||
' return createElement("div", {}, "hello");', | ||
'};' | ||
].join('\n'), | ||
parser: 'babel-eslint', | ||
errors: [{ | ||
message: 'Component definition is missing display name' | ||
}] | ||
}, { | ||
code: [ | ||
'import React from "react";', | ||
'const createElement = React.createElement;', | ||
'export default (props) => {', | ||
' return createElement("div", {}, "hello");', | ||
'};' | ||
].join('\n'), | ||
parser: 'babel-eslint', | ||
errors: [{ | ||
message: 'Component definition is missing display name' | ||
}] | ||
}] | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this prevent
import { createElement } from 'react';
from being detected?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.