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.

Sign up for GitHub

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

Jump to bottom

Fix no-this-in-sfc for class properties #1995

Merged
merged 4 commits into from Sep 29, 2018
Merged

Fix no-this-in-sfc for class properties #1995

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68d6384
Fix `no-this-in-sfc` for class properties
sergei-startsev Sep 24, 2018
08e9d0b
Extract util functions to AST utils
sergei-startsev Sep 27, 2018
fee8c36
Remove redundant `require('babel-eslint')`
sergei-startsev Sep 28, 2018
e779b00
Minor adjustments
sergei-startsev Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 12 additions & 10 deletions lib/util/Components.js
View file
Open in desktop
Expand Up @@ -456,15 +456,16 @@ function componentRule(rule, context) {
let scope = context.getScope();
while (scope) {
const node = scope.block;
const isClass = node.type === 'ClassExpression';
const isFunction = /Function/.test(node.type); // Functions
const isArrowFunction = node.type === 'ArrowFunctionExpression';
let functionScope = scope;
let enclosingScope = scope;
if (isArrowFunction) {
functionScope = utils.getParentFunctionScope(scope);
enclosingScope = utils.getArrowFunctionScope(scope);
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it could be const enclosingScope = isArrowFunction ? utils.getArrowFunctionScope(scope) : scope;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing, fixed

}
const methodNode = functionScope && functionScope.block.parent;
const isMethod = methodNode && methodNode.type === 'MethodDefinition'; // Classes methods
const enclosingScopeType = enclosingScope && enclosingScope.block.type;
const enclosingScopeParent = enclosingScope && enclosingScope.block.parent;
const isClass = enclosingScopeType === 'ClassDeclaration' || enclosingScopeType === 'ClassExpression';
const isMethod = enclosingScopeParent && enclosingScopeParent.type === 'MethodDefinition'; // Classes methods
const isArgument = node.parent && node.parent.type === 'CallExpression'; // Arguments (callback, etc.)
// Attribute Expressions inside JSX Elements (<button onClick={() => props.handleClick()}></button>)
const isJSXExpressionContainer = node.parent && node.parent.type === 'JSXExpressionContainer';
Expand All @@ -482,15 +483,16 @@ function componentRule(rule, context) {
},

/**
* Get a parent scope created by a FunctionExpression or FunctionDeclaration
* @param {Scope} scope The child scope
* @returns {Scope} A parent function scope
* Get an enclosing scope used to find `this` value by an arrow function
* @param {Scope} scope Current scope
* @returns {Scope} An enclosing scope used by an arrow function
*/
getParentFunctionScope(scope) {
getArrowFunctionScope(scope) {
scope = scope.upper;
while (scope) {
const type = scope.block.type;
if (type === 'FunctionExpression' || type === 'FunctionDeclaration') {
if (type === 'FunctionExpression' || type === 'FunctionDeclaration'
Copy link
Member

Choose a reason for hiding this comment

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

i think we have a utility function for "is a function"

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 didn't find isFunction, only isFunctionLikeExpression in ast.js, so extracted some util functions for reusing in Components.js

|| type === 'ClassDeclaration' || type === 'ClassExpression') {
return scope;
}
scope = scope.upper;
Expand Down
20 changes: 20 additions & 0 deletions tests/lib/rules/no-this-in-sfc.js
View file
Open in desktop
Expand Up @@ -16,6 +16,8 @@ const ERROR_MESSAGE = 'Stateless functional components should not use this';
const rule = require('../../../lib/rules/no-this-in-sfc');
const RuleTester = require('eslint').RuleTester;

require('babel-eslint');
Copy link
Contributor

Choose a reason for hiding this comment

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

@ljharb do you know if this is really necessary? Not all of the test files that use this parser actually have the require.

Copy link
Member

Choose a reason for hiding this comment

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

if you run those test files in isolation, do they pass?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, they do - just checked with ESLint 3, 4, and 5. could be an artifact from an even earlier time.

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 don't see any differences after removing require('babel-eslint'). See the proof #2004 . Also removed it here.


const parserOptions = {
ecmaVersion: 2018,
sourceType: 'module',
Expand Down Expand Up @@ -119,6 +121,24 @@ ruleTester.run('no-this-in-sfc', rule, {
};
}
}`
}, {
code: `
class Foo {
bar = () => {
this.something();
return null;
};
}`,
parser: 'babel-eslint'
}, {
code: `
class Foo {
bar = () => () => {
this.something();
return null;
};
}`,
parser: 'babel-eslint'
}],
invalid: [{
code: `
Expand Down