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

Valid self reference jsx element. #905

Merged
merged 3 commits into from
Oct 18, 2016
Merged

Valid self reference jsx element. #905

merged 3 commits into from
Oct 18, 2016

Conversation

xeodou
Copy link
Contributor

@xeodou xeodou commented Oct 12, 2016

Please check the information here #839

I'm trying to include xo in one of our projects, i hit the issue exactly the same as @pfhayes . And after dig into the codebase, i found we have a self reference linked jsx element and that cause the issue, Like this

class Foo extends React.createClass {
   render() {
       return <this.Bar  className='demo' />
   }
}

That's cause the issue. I don't think it's a best practice, but at least it shouldn't crash the workflow.

@@ -81,6 +81,17 @@ ruleTester.run('forbid-component-props', rule, {
].join('\n'),
options: [{forbid: ['style', 'foo']}],
parserOptions: parserOptions
}, {
code: [
'var First = React.createClass({',
Copy link
Member

Choose a reason for hiding this comment

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

could you also add examples with an SFC, and with a class-based component?

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, SFC is short for self-closing component right ?

Copy link
Member

Choose a reason for hiding this comment

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

no - it's for "stateless functional component" - ie, a function that takes props and returns JSX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, Thanks. I know 'stateless functional component' , just have no idea what does SFC meaning. Thanks for point out

' }',
'});'
].join('\n'),
options: [{forbid: ['style', 'foo']}],
Copy link
Member

Choose a reason for hiding this comment

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

hmm - why would "foo" lowercase catch "Foo", which is a distinct property in JS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check the test.

Copy link
Contributor Author

@xeodou xeodou Oct 12, 2016

Choose a reason for hiding this comment

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

I think i just forget remove the line options: [{forbid: ['style', 'foo']}], it shouldn't relate to forbid option.

@xeodou
Copy link
Contributor Author

xeodou commented Oct 12, 2016

hi @ljharb I just made some changes, Are there what you expect ?

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! I'll let other collabs take a look first tho.

@EvHaus
Copy link
Collaborator

EvHaus commented Oct 12, 2016

LGTM

@xeodou
Copy link
Contributor Author

xeodou commented Oct 13, 2016

Cool. Thanks @ljharb and @EvNaverniouk

@xeodou
Copy link
Contributor Author

xeodou commented Oct 13, 2016

Hi @ljharb When can you merge the pull request ?

@xeodou
Copy link
Contributor Author

xeodou commented Oct 18, 2016

Hi @ljharb @EvNaverniouk , Do we have any other issue here for merge the PR ?

@ljharb ljharb merged commit 99cec3e into jsx-eslint:master Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants