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

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

merged 4 commits into from Sep 29, 2018

Conversation

sergei-startsev
Copy link
Contributor

@sergei-startsev sergei-startsev commented Sep 24, 2018 •
edited by ljharb

The PR fixes #1960 - no-this-in-sfc rule for class properties.

Class properties example:

class Foo {
  bar = () => {
    this.something();
    return null;
  };
}

The corresponding AST: link.

ljharb
ljharb reviewed Sep 25, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM overall

lib/util/Components.js Outdated
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

ljharb
ljharb approved these changes Sep 28, 2018
View reviewed changes
lib/util/Components.js Outdated
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

@ljharb ljharb added the bug label Sep 28, 2018
alexzherdev
alexzherdev reviewed Sep 28, 2018
View reviewed changes
tests/lib/rules/no-this-in-sfc.js Outdated
@@ -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.

EvHaus
EvHaus approved these changes Sep 28, 2018
View reviewed changes
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

LGTM as well

@ljharb ljharb merged commit b77be96 into jsx-eslint:master Sep 29, 2018
@AnabelSalomone AnabelSalomone mentioned this pull request Oct 22, 2018
Closed
@renovate renovate bot mentioned this pull request Dec 28, 2018
Merged
1 task
@renovate-bot renovate-bot mentioned this pull request Dec 28, 2018
Closed
1 task
This was referenced Dec 28, 2018
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Open
Closed
Merged
Closed
Open
Open
Merged
This was referenced Jan 4, 2019
Merged
Closed
Open
Merged
Closed
Merged
Closed
Merged
Merged
Closed
Closed
Merged
Closed
Merged
Closed
Merged
Merged
Merged
@ghost ghost mentioned this pull request Jan 12, 2019
Merged
1 task
This was referenced Jan 13, 2019
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 17, 2019
Open
Open
This was referenced Sep 22, 2019
Open
Open
@snyk-bot snyk-bot mentioned this pull request Sep 30, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@alexzherdev alexzherdev alexzherdev left review comments

@EvHaus EvHaus EvHaus approved these changes

@lencioni lencioni Awaiting requested review from lencioni

@yannickcr yannickcr Awaiting requested review from yannickcr

Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting unrelated (react/no-this-in-sfc) violation error
4 participants
@sergei-startsev @ljharb @alexzherdev @EvHaus