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

update eslint, use babel-eslint, eslint-plugin-react, fixes #1370

Closed
wants to merge 1 commit into from

Conversation

hzoo
Copy link
Contributor

@hzoo hzoo commented May 22, 2015

While working on babel/babel-eslint#108, @wincent mentioned I could check out react-native for examples of flow type usage.

In order to test I updated eslint": "0.21.2", fix deprecated rules, add config to support es6/jsx/react, add babel-eslint. I figured I could do a PR with some fixes (mostly unused parameters, quotes, semicolons etc) to see if it's wanted?

You should be able to then use "lint": "./node_modules/.bin/eslint Examples Libraries" (I didn't fix anything in Libraries yet)

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 22, 2015
@a2 a2 assigned vjeux May 22, 2015
@vjeux vjeux assigned jaredly and unassigned vjeux May 22, 2015
@@ -129,7 +146,7 @@
"no-undef": 2, // disallow use of undeclared variables unless mentioned in a /*global */ block
"no-undefined": 0, // disallow use of undefined variable (off by default)
"no-undef-init": 1, // disallow use of undefined when initializing variables
"no-unused-vars": [1, {"vars": "all", "args": "none"}], // disallow declaration of variables that are not used in the code
// "no-unused-vars": [1, {"vars": "all", "args": "none"}], // disallow declaration of variables that are not used in the code
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

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 should of made a comment for that sorry (or just remove it).

It's replaced with "react/jsx-uses-vars": 1, in line 54 because of issues with jsx/eslint. ESLint isn't supporting it so it's recommended to use eslint-plugin-react

ESLint supports only the JSX syntax not the semantic of react. http://eslint.org/blog/2015/03/eslint-0.17.0-released/

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-uses-vars.md

https://github.com/eslint/eslint/search?q=no-unused-vars+for+jsx&type=Issues&utf8=%E2%9C%93

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know! However, it looks like this still shouldn't be disabled. The jsx-uses-vars docs say:

This rule has no effect if the no-unused-vars rule is not enabled.

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 ok - good to know xd - will add it back. Probably need to use these rules at some point instead of just recommending it.

@hzoo hzoo force-pushed the update-eslint branch from 8deaa6f to a3890b4 Compare May 22, 2015 20:12
@hzoo
Copy link
Contributor Author

hzoo commented May 23, 2015

And from Travis - not sure what can be done about this?

/Users/travis/build/facebook/react-native/node_modules/jstransform/node_modules/commoner/test/source/widget/share.js: WidgetShare
Duplicate module provider
/Users/travis/build/facebook/react-native/node_modules/babel-eslint/node_modules/babel-core/node_modules/regenerator/node_modules/commoner/test/source/widget/share.js:

@hzoo
Copy link
Contributor Author

hzoo commented May 23, 2015

@jaredly Oops I noticed (you probably did as well?) there's already #259 which does the same as this other than not including fixes for lint issues.

@jaredly
Copy link
Contributor

jaredly commented May 23, 2015

Yup :) I've integrated the .eslintrc & package.json parts of it into a commit internally (in conjunction with #259). This will be synced up here soon.

I'm going to hold off on the lint fixes for now -- the extra arguments to functions are good for documentation (how will this function be called) even if they aren't used, and I think the convention is to double-quote jsx params

Thanks for the work!

@hzoo
Copy link
Contributor Author

hzoo commented May 23, 2015

Ah ok!
For the quotes I guess that's what jsx-quotes enforces correctly.
And I guess the extra args shouldn't be warning since "no-unused-vars": [1, {"vars": "all", "args": "none"}] shouldn't check for arguments..

@hzoo hzoo force-pushed the update-eslint branch from a3890b4 to 2291c9a Compare May 23, 2015 20:25
@hzoo hzoo force-pushed the update-eslint branch from 2291c9a to de0d2a5 Compare May 23, 2015 20:31
@ccheever
Copy link
Contributor

the packager now uses babel and the linter uses babel-eslint. is there anything else that needs to be done with this?

@hzoo
Copy link
Contributor Author

hzoo commented May 30, 2015

No this just fixed a few of the lint errors but that can be done seperately - will close.

@hzoo hzoo closed this May 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants