-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added eslint config #9
base: master
Are you sure you want to change the base?
Conversation
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.
First and foremost, thanks for taking the time to contribute. It's greatly appreciated! This seems like a great starter for when the frontend and backend are bundled together, I'm just not sure if that's the goal here.
With it just being an express backend API, I believe anything related to React can safely be removed.
.eslintrc.js
Outdated
"airbnb", | ||
"prettier", | ||
"prettier/react" |
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.
Do we need React eslint rules in this project? My understanding was that it's just an express server sending over data.
"parser": "babel-eslint", | ||
"parserOptions": { | ||
"ecmaVersion": 8, |
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.
In the package.json
, we should additionally enforce the JS version with an engines key. That way it's clear why we are parsing at version 8.
.eslintrc.js
Outdated
"browser": true, | ||
"node": true, | ||
"jquery": true |
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 would remove jquery as it's not currently a dependency.
"test": true, | ||
"expect": true, | ||
"afterEach": true, |
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.
Do we also need to add in beforeEach
and describe
?
…ibe to jest globals.
@mtliendo Hi Michael What engine specs are you looking for? I went with All the other comment recommendations should be taken care of. Share your thoughts. |
I don't think we should support unmaintained versions of node. It's safe to use |
@mtliendo I had a chance to swap out the node version. Happy to help. -Cheers |
Quick question. Are you considering GraphQL for any part of the API? |
This is a hybrid config built on Wes Bos' eslint configuration. I have been using it on React, Express, and GraphQL projects for the last 6 months or so.