-
Notifications
You must be signed in to change notification settings - Fork 904
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 configuration #52
Conversation
@@ -34,4 +34,4 @@ const withPods = { | |||
ios: ios.pod, | |||
}; | |||
|
|||
module.exports = {flat, nested, withExamples, withPods}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we go with non-fb style of extra spacing in destructuring statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back to the roots! https://github.com/rnpm/rnpm/blob/master/src/config/android/index.js#L110
On a serious note: I don't have preference here. Just sticking to whatever ESLint preset is recommending. I guess we can always change that in case we agree on a general preset for all RNCommunity projects - right now, it's pretty much up to project maintainers.
let commands = acc.commands; | ||
let platforms = acc.platforms; | ||
let haste = acc.haste; | ||
let { commands, platforms } = acc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haste
could be retrieved here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this would cause another error: using "let" where "haste" is not re-assigned. Wanted to avoid double-destructing statement for the sake of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good generally, but there's just too many changes to review properly :D
Thanks @thymikee - that's right! Luckily, most of the changes were performed automatically by ESLint, so I am quite confident they will not cause major issues. |
@@ -10,8 +10,6 @@ | |||
import type { ContextT } from '../core/types.flow'; | |||
import type { CommandLineArgs } from './bundleCommandLineArgs'; | |||
|
|||
('use strict'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably shouldn't remove 'use strict'
if you want your scripts to run in strict mode as you're not using ES6 modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already handled by Babel plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee which plugin does it? according to babel's docs, the sourceType
defaults to "script"
and will be "module"
only if you're using import/export
. so since the code here uses require
and not import/export
, it shouldn't insert 'use strict'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what's used: @babel/plugin-transform-strict-mode
. Does it matter which sourceType we use when having that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee if you use that it shouldn't matter I think, but where is it specified? i couldn't find it here: https://github.com/react-native-community/react-native-cli/blob/master/packages/local-cli/babel.config.js#L4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind. it's newly added in the diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I requested it :D glad you're up to date now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍
This PR adds ESLint configuration with Prettier and automatically fixes most of the issues. Some of the changes had to be done manually.
ESLint is now part of our testing pipeline.
3 warnings remaining for disabled tests, to be implemented in upcoming PRs.
I excluded "debugger-ui" folder from ESLint based on historical issues we had with running ESLint in Haul on that folder. This runs in browser context "as-is" without transpilation and should not be changed.
"templates" folder will be gone in favour of the one present in React Native master (to be done as a next PR).