Fixes eslint config #90
Conversation
.eslintrc.js
Outdated
| @@ -1,6 +1,9 @@ | |||
| module.exports = { | |||
| 'extends': 'airbnb', | |||
| 'rules': { | |||
| 'react/jsx-filename-extension': [1, { "extensions": [".js", ".jsx"] }], | |||
There was a problem hiding this comment.
Do we want to allow files with both these extensions to have JSX? Is this for tests?
Also, could you use "warn" instead of "1"?
There was a problem hiding this comment.
Well we should not have .jsx extensions right? This is for tests.
There was a problem hiding this comment.
Sorry, if you're saying we shouldn't have .jsx extensions, shouldn't we only be allowing .js extensions then?
There was a problem hiding this comment.
Isn't not having .jsx extensions the status quo for js at Yelp?
There was a problem hiding this comment.
@jeffrey-xiao @adityavohra7 What would the recommendation be for a test that has jsx? .jsx or .js?
I believe @jeffrey-xiao is suggesting tests can have jsx in .js files for tests, hence the whitelisted extension. This generalized rule does seem to be ill suited for components and containers where we want to enforce jsx to be only in .jsx files.
There was a problem hiding this comment.
We've gone with .js everywhere at Yelp because of this reasoning. I personally think allowing two file extensions is confusing, but it isn't that big of a deal either way!
package.json
Outdated
| @@ -25,7 +25,7 @@ | |||
| "eslint-config-airbnb": "^15.0.1", | |||
| "eslint-loader": "^1.6.1", | |||
| "eslint-plugin-import": "^2.2.0", | |||
| "eslint-plugin-jsx-a11y": "^6.0.2", | |||
| "eslint-plugin-jsx-a11y": "^5.1.1", | |||
There was a problem hiding this comment.
Could you clarify why you're down-pinning this?
There was a problem hiding this comment.
Related to this github issue: https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/232
There was a problem hiding this comment.
Cool! Did you run this command to update the deps?
(
export PKG=eslint-config-airbnb;
npm info "$PKG@latest" peerDependencies --json | command sed 's/[\{\},]//g ; s/: /@/g' | xargs npm install --save-dev "$PKG@latest"
)
There was a problem hiding this comment.
No, but I think that's the latest version of eslint-plugin-jsx-a11y that's compatible with eslint-config-airbnb
There was a problem hiding this comment.
@jeffrey-xiao you can remove this portion from the PR, @sjhworkman separated this into another PR that is now merged
* 'master' of https://github.com/Yelp/beans: Save multiple participants for a meeting into db + refactoring (Yelp#100) method to get a dictionary of {user_pair:previous meeting count} (Yelp#98) downgrade react-router to v3.0.0 and a11y to v5.1.1 (Yelp#99) Revert eslint-plugin-jsx-a11y version to 5.1.1 to deal webpack errors (Yelp#97) Only disallow a pair if they matched earlier for the same subscription (Yelp#94) Devme (Yelp#92) Import PropTypes from prop-types package instead of react Created new module for matching. Renamed match.py to pair_match.py
webpack.config.js
Outdated
| @@ -26,13 +26,13 @@ module.exports = { | |||
| rules: [ | |||
| { | |||
| use: 'eslint-loader?{fix: true}', | |||
| test: /\.jsx?$/, | |||
| test: /\.js?$/, | |||
There was a problem hiding this comment.
This should just be /\.js$/. Same for the others.
webpack.config.js
Outdated
| @@ -42,7 +42,7 @@ module.exports = { | |||
| contentBase: './', | |||
| }, | |||
| resolve: { | |||
There was a problem hiding this comment.
I think you can remove this entirely. The defaults handle this.
package-lock.json
Outdated
| @@ -0,0 +1,7391 @@ | |||
| { | |||
There was a problem hiding this comment.
Delete this file? Or add it in another CR?
|
@adityavohra7 Do we still consider this to be the appropriate way to move forward? |
|
Closing this fix as it is out of date with the current master. Will merge in changes and submit in a new pull request. |
No description provided.