-
Notifications
You must be signed in to change notification settings - Fork 322
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
Updated Babel, Webpack, React, Eslint, etc to latest & Clean up linting errors #719
base: master
Are you sure you want to change the base?
Conversation
Looks like there was an issue with the coverage plugin. Will look into that and ammend this PR once I fix. |
I fixed the issue with the travis CLI. It was due to an updated dependency. I also found and fixed an issue with the babel test file due to the eslinting. |
@@ -1,5 +1,5 @@ | |||
{ | |||
"presets": ["airbnb", "stage-0"], |
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.
I'd like to keep this as-is
"ecmaFeatures": { | ||
"modules": true, | ||
"jsx": true | ||
}, | ||
"rules": { |
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.
same here, can we revert this file to include only the single rule. We can perhaps add individual overrides per line.
no-param-reassign
can be easily fixed by renaming all the reducers from obj
to acc
(acc
is whitelisted as an acceptable param that can be reassigned)
"react/jsx-filename-extension": 0, | ||
"react/forbid-prop-types": 0, | ||
}, | ||
"parser": "babel-eslint" |
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.
don't think we need this, but we may need it for some stage-0 features. I'd happily .eslintignore anything that is stage0 though, since it's supposed to be experimental
@@ -1,5 +1,9 @@ | |||
# Changelog | |||
|
|||
## 1.0.0 |
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.
oh no not yet.
"flux": "2.1.1", | ||
"is-promise": "2.1.0", | ||
"transmitter": "3.0.1" | ||
"flux": "^3.1.3", |
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.
let's downgrade this back down to 2.1.1, the improvements past that version just bloat the library unnecessarily especially for what Alt is using the flux lib for. And the new add-ons in 3 aren't worth the bump.
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/goatslacker/alt.git" | ||
"url": "https://github.com/ryank311/alt.git" |
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.
??
@@ -58,6 +64,7 @@ | |||
"clean": "rimraf lib", | |||
"coverage": "npm run transpile-cover && babel-node node_modules/.bin/istanbul cover node_modules/.bin/_mocha -- -u exports -R tap --require test/babel test", | |||
"lint": "eslint src components", | |||
"lint-fix": "eslint --fix src test scripts", |
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.
Nice! 👍
}, | ||
|
||
neverFetchUsers: { | ||
remote, | ||
local: () => false, | ||
local: () => { return false }, |
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.
what's up with all these changes?
this.props.alt.stores.store.listen(state => this.setState(state)) | ||
} | ||
componentWillMount() { | ||
this.props.alt.stores.store.listen((state) => { return this.setState(state) }) |
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.
weird spacing
Can you rebase so that the commits are split up into units of work?
We should actually remove the checked-in build or fix webpack so that it's more deterministic. |
Hey, I upgraded most of the dependencies to latest versions and cleaned up all the linting errors across all the files. I tried to keep it as close to the original source where applicable, and if I wasn't sure on a rule, I just commented it out with /eslint-disable-line. All tests should pass with this PR and build appears to be good with the latest webpack.
-Ryan