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

(wip) added eslint #280

Merged
merged 7 commits into from
May 4, 2016
Merged

(wip) added eslint #280

merged 7 commits into from
May 4, 2016

Conversation

nfcampos
Copy link
Collaborator

@nfcampos nfcampos commented May 2, 2016

@gaearon want to review the config?

I still need to fix most of the files to conform to the config

closes #274

},
"rules": {
"no-underscore-dangle": ["error", { "allow": ["__REACT_HOT_LOADER__"] }],
"jsx-quotes": ["error", "prefer-single"]
Copy link
Owner

@gaearon gaearon May 2, 2016

Choose a reason for hiding this comment

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

I’m fine with using double quotes everywhere. (for JSX)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

using double quotes for jsx and single quotes for everything else isn't weird?

Copy link
Owner

Choose a reason for hiding this comment

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

It’s weird but apparently it’s what everyone else is doing these days 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

double quotes it is then 😄

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

Looks good. I left a few comments.
You can try using eslint --fix . to get some of these fixed.

@nfcampos
Copy link
Collaborator Author

nfcampos commented May 2, 2016

Yeah --fix helps at least with the semicolons, adding semicolons in hundreds of lines manually would have been terrible

@nfcampos
Copy link
Collaborator Author

nfcampos commented May 2, 2016

@gaearon you wrote some of the files without const, import, stuff like that, any reason to keep them like that, since we're building everything with babel? I need to either change them or change the eslint rules

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

If we use ES modules everywhere, we should add the commonjs compat plugin so that react-hot-loader/webpack and react-hot-loader/babel keep working as they are.

@nfcampos
Copy link
Collaborator Author

nfcampos commented May 2, 2016

add the plugin to babel?

@nfcampos nfcampos changed the title added eslint (wip) added eslint May 2, 2016
@gaearon
Copy link
Owner

gaearon commented May 2, 2016

Yeah, this one: https://www.npmjs.com/package/babel-plugin-add-module-exports. Not sure if it works well though. Never tried it.

@nfcampos
Copy link
Collaborator Author

nfcampos commented May 2, 2016

hmm, not worth it then I think. basically I'm getting a bunch of error 'use strict' is unnecessary inside of modules from this rule http://eslint.org/docs/rules/strict + airbnb config adding parserOptions: {sourceType: 'module' } to the eslint config. So all of the non-es6 modules files in src/ are being interpreted as being modules and thus not needing 'use strict'. but i dont think it's possible to disable parserOptions on a single file. should i just disable the strict rule?

@gaearon
Copy link
Owner

gaearon commented May 2, 2016

I’m a bit confused. I meant that we can use ES modules everywhere in the code. We just need to make sure that the CommonJS compat code is generated as part of Babel compilation for entry points like /babel and /webpack. Does this make sense? Or are you saying this makes ESLint fail?

@nfcampos
Copy link
Collaborator Author

nfcampos commented May 2, 2016

what i'm saying is that it currently works great the way it's written so instead of changing everything to es6 modules just to appease eslint it's easier to selectively disable the offending rule, no?

@nfcampos nfcampos force-pushed the eslint branch 2 times, most recently from 0282793 to 053157a Compare May 2, 2016 23:15
@gaearon
Copy link
Owner

gaearon commented May 2, 2016

what i'm saying is that it currently works great the way it's written so instead of changing everything to es6 modules just to appease eslint it's easier to selectively disable the offending rule, no?

Works for me 👍

@nfcampos
Copy link
Collaborator Author

nfcampos commented May 3, 2016

Alright, I've finished fixing all the linting errors. Give it a look if you want, I think/hope I didn't break anything. Let's merge this soon to avoid merge conflicts

if (React.Children.count(props.children) !== 1) {
return new Error(`Invalid prop "children" supplied to AppContainer. Expected a single React element with your app’s root component, e.g. <App />.`);
return new Error('Invalid prop "children" supplied to AppContainer.' +
Copy link
Owner

Choose a reason for hiding this comment

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

There’s a missing space after the period in the error message. The sentences will collide.
Also a style nit: let’s put a newline right after the opening paren.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch

@gaearon
Copy link
Owner

gaearon commented May 3, 2016

The error message needs to be fixed but the rest is up to your judgement.
:shipit:

@nfcampos nfcampos merged commit 0ed3719 into next May 4, 2016
@nfcampos nfcampos deleted the eslint branch May 4, 2016 00:55
@gaearon gaearon mentioned this pull request May 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants