Update LPLs eslint config to 3.0.1 #26
Conversation
be/package.json
Outdated
| @@ -1,13 +1,13 @@ | |||
| { | |||
| "name": "@veritext-replacement-template/be", | |||
There was a problem hiding this comment.
Isn't this being addressed in #21 ?
There was a problem hiding this comment.
@vanderhoop correct, I implemented it here so I could continue working on a separate branch. Whatever branch gets merged first I will make sure it is updated.
There was a problem hiding this comment.
merged #21 into main so this is no longer an issue. Updated PR body
fe/babel.config.json
Outdated
| @@ -5,9 +5,10 @@ | |||
| { | |||
| "env": { | |||
| "useBuiltIns": "usage", | |||
| "corejs": "3.19" | |||
There was a problem hiding this comment.
This is technically a downgrade. Any particular reason we'd downgrade?
There was a problem hiding this comment.
@vanderhoop thanks, fixed this
be/.gitignore
Outdated
| test.json |
There was a problem hiding this comment.
I think we'll want to remove this from the diff.
be/.eslintrc.cjs
Outdated
| module.exports = { | ||
| root: true, | ||
| env: { | ||
| node: true, | ||
| browser: true, | ||
| es6: true, | ||
| }, | ||
| extends: [ | ||
| "prettier", | ||
| "eslint:recommended", |
There was a problem hiding this comment.
@oroth8 Small thing, but I think your editor's default settings might be setting you up with 4 spaces per tab. The source files are mostly 2-spaces, so we might want to that.
Normally, I wouldn't comment on lint stuff, but early in the project, it makes sense to get our editor configs' aligned so that future PR diffss don't end up with a bunch of whitespace noise.
There was a problem hiding this comment.
@vanderhoop LPL uses a 2-tab width across all projects:
this is what the .prettierrc file at the root of our project uses
There was a problem hiding this comment.
I think tabWidth: 2 means 2 spaces per tab? See the be/src/controllers/users.controller.js file and other source files below for cases where the indentation is 2 spaces.
I suspect the reason this particular file isn't taking on the prettier defaults is that the path pattern isn't included in launchpad's prettier config. (Will check)
There was a problem hiding this comment.
I suspect the reason this particular file isn't taking on the prettier defaults is that the path pattern isn't included in launchpad's prettier config. (Will check)
I checked, and it's not the .prettierrc or .prettierignore file that's causing this file to go unformatted.
I think it's this line in the package.json, which doesn't include *.cjs files:
"be/**/*.{js,json}": [
"prettier --write",
"eslint --fix --max-warnings=0"
]
There was a problem hiding this comment.
I think my vscode plugin workspace settings were overriding the .prettierrc file at the root. Anyways I added cjs to the lint-staged script and also reran prettier using their --config flag so I knew for a fact it was using .prettierrc
| @@ -10,7 +10,7 @@ notifications: | |||
| addons: | |||
| apt: | |||
| packages: | |||
| - libgconf-2-4 | |||
| - libgconf-2-4 | |||
There was a problem hiding this comment.
Inner monologue: Any chance we want to add linting to travis CI? It could avoid the manual Nit: * comments in PRs, and let CI do the job for us. Just a thought
There was a problem hiding this comment.
@vanderhoop I was thinking of creating a github action and add linting there in a separate PR. Reason being, Im not sure if we will be able to use travisCI with veritext so it might be better to use github's
There was a problem hiding this comment.
Yeah, now that I'm thinking about it, we should hold off until fully in Veritext's GitHub and they've given us a Jenkins file we can add scripts to.
There was a problem hiding this comment.
Yeah, now that I'm thinking about it, we should hold off until fully in Veritext's GitHub and they've given us a Jenkins file we can add scripts to.
Reason being the work would be redundant, and we're gonna have to switch to their repo relatively soon
There was a problem hiding this comment.
This file is a woopsy in the diff, as nothing in node_modules either at the root or in lower directories should be in version control. I think deleting this file from version control and adding the following two lines to the .gitignore file would address this:
# (rest of .gitignore)
be/node_modules/
fe/node_modules/There was a problem hiding this comment.
also ran git rm -r on:
node_modules
be/node_modules
fe/node_modules
package.json
Outdated
| @@ -44,7 +44,8 @@ | |||
| ], | |||
| "fe/src/**/*.{json,scss}": "prettier --write", | |||
| "be/**/*.{js,json}": [ | |||
There was a problem hiding this comment.
See comment above about adding cjs files
| "be/**/*.{js,json}": [ | |
| "be/**/*.{js,json,cjs}": [ |
Changes
This PR updates our eslint config to uses LPL's latest eslint-config.
feeslintrcfile inbefor separate configurationbelint-staged scriptLots of changed files but most of them are due to the linting changes that are implemented.
Files to review in this PR are: