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

add eslint rules from fbjs #202

Closed
wants to merge 5 commits into from
Closed

Conversation

knowbody
Copy link
Contributor

@knowbody knowbody commented Sep 1, 2015

As @zpao suggested (reference #58) here is the new PR with the rules from facebook/fbjs#49.

I'll still need some guidance on what should be Relay specific.

At the moment with the current fbjs rules there is a lot of errors on no-undef (example: $FlowIssue, $FixMe, $Enum and also when defining Flow types, this is related to babel-eslint/known-issues - babel-eslint#130 and babel-eslint#132)

@josephsavona @zpao what are your thoughts?

node_modules/
lib/
scripts/
# for disable website but should be done as well
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean something like this?

# TODO: Enable ESLint for website.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yungsters haha! yeah that's what I meant, not sure my comment is even in English lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh it was supposed to be "for now... "

@yungsters
Copy link
Contributor

Here are the "undefined" identifiers:

  • $Enum
  • $FixMe
  • $FlowFixMe
  • $FlowIssue
  • Iterator
  • IteratorResult
  • ReactClass
  • ReactElement

I think we should just define these as globals in .eslintrc for now.

@zpao
Copy link
Member

zpao commented Sep 1, 2015

Yea, you can define them here for now. They should make their way back to fbjs (and there are probably other ones that we should just copy over from flow). We have them defined internally, I just went for the minimal sync.

@knowbody knowbody force-pushed the add-new-eslint branch 2 times, most recently from 5bb3986 to ddb827a Compare September 1, 2015 23:22
@knowbody
Copy link
Contributor Author

knowbody commented Sep 1, 2015

@zpao should be good

@knowbody knowbody changed the title [WIP] add eslint rules from fbjs add eslint rules from fbjs Sep 2, 2015
scripts/
# for disable website but should be done as well
website/
*.md
Copy link
Contributor

Choose a reason for hiding this comment

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

When you update the comment, can you also add a newline at the end of the file? Makes life easier (internally, we have lints that try to automatically add these).

@knowbody
Copy link
Contributor Author

knowbody commented Sep 4, 2015

@yungsters done, the tests are failing but not sure why, haven't done any changes to src/

@zpao
Copy link
Member

zpao commented Sep 5, 2015

👍 thanks! We'll go through the land internally and merge out process, should be all wrapped up soon.

@ghost ghost closed this in a7e8108 Sep 9, 2015
steveluscher pushed a commit that referenced this pull request Sep 18, 2015
Summary: As @​zpao suggested (reference #58) here is the new PR with the rules from [facebook/fbjs#49](facebook/fbjs#49).

I'll still need some guidance on what should be Relay specific.

At the moment with the current fbjs rules there is a lot of errors on [no-undef](http://eslint.org/docs/rules/no-undef.html) (example: $FlowIssue, $FixMe, $Enum and also when defining Flow types, this is related to [babel-eslint/known-issues](https://github.com/babel/babel-eslint#known-issues) - [babel-eslint#130](babel/babel-eslint#130) and [babel-eslint#132](babel/babel-eslint#132))

@​josephsavona @​zpao what are your thoughts?
Closes #202

Reviewed By: @josephsavona

Differential Revision: D2417828
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants