Skip to content

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
.eslintrc Outdated
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
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..

@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