Skip to content
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

Run Prettier and fix ESLint complaints #28

Merged
merged 5 commits into from Dec 5, 2021
Merged

Conversation

alexnguyenn
Copy link
Member

As title

@alexnguyenn
Copy link
Member Author

@gwwatkin I turned off @typescript-eslint/ban-ts-comment which interferes with @ts-ignore comment in BaseTemplate.tsx. If you know how to fix it feel free to do so.

Also here are the last warnings I am having for LatticeView.tsx:

157:35  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion
157:35  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion
163:34  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion
163:34  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion

Since they are warnings they won't fail any CI we setup later on, but if you can fix it go ahead.

@gwwatkin
Copy link
Member

gwwatkin commented Dec 5, 2021

Get this error running npm run lint:fix:

> ls-ui-react-app@0.1.0 lint:fix
> eslint --fix src/**/*.{js,jsx,ts,tsx}


Oops! Something went wrong! :(

ESLint: 7.32.0

No files matching the pattern "src/**/*.js" were found.
Please check for typing mistakes in the pattern.

Which is strange because, npx eslint --fix src/**/*.{js,jsx,ts,tsx} runs as expected

@gwwatkin
Copy link
Member

gwwatkin commented Dec 5, 2021

Ok found the problem the npx eslint --fix ./src/**/*.{js,jsx,ts,tsx} also doesn't work, its npx eslint --fix ./src/**/*.{ts,tsx} that works

@gwwatkin
Copy link
Member

gwwatkin commented Dec 5, 2021

Tried this: import-js/eslint-plugin-import#1645 (comment)
also doesn't work

@gwwatkin
Copy link
Member

gwwatkin commented Dec 5, 2021

This is the only thing that fixes it, though it excludes js and jsx files from liniting

diff --git a/package.json b/package.json
index 2828b95..8353fac 100644
--- a/package.json
+++ b/package.json
@@ -34,8 +34,8 @@
     "build": "react-scripts build",
     "test": "react-scripts test",
     "eject": "react-scripts eject",
-    "lint": "eslint src/**/*.{js,jsx,ts,tsx}",
-    "lint:fix": "eslint --fix src/**/*.{js,jsx,ts,tsx}"
+    "lint": "eslint src/**/*.{ts,tsx}",
+    "lint:fix": "eslint --fix src/**/*.{ts,tsx}"
   },
   "browserslist": {
     "production": [

@gwwatkin
Copy link
Member

gwwatkin commented Dec 5, 2021

Not that it's a big problem to excluding js and jsx from linting, but it would be nice to combine it with something that forbids js and jsx. Tried the "allowJs": false, option in tsconfig.json but it also forbids importing js which is going to be a problem with modules like JQuery.

Also if we do end up adding some plain js we can always just re enable the option.

Comment on lines +37 to +38
"lint": "eslint --no-error-on-unmatched-pattern src/**/*.{js,jsx,ts,tsx}",
"lint:fix": "eslint --no-error-on-unmatched-pattern --fix src/**/*.{js,jsx,ts,tsx}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it turns out I could just add this option here, sorry for the noise in the PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem, nice to see that we have a fix for this.

Copy link
Member

@gwwatkin gwwatkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some fixes

  • --no-error-on-unmatched-pattern
  • null assertion warnings
  • removed the ts-ignore directive

Comment on lines +12 to +13
const windowWidth = $(window).width() ?? 1000 // Default width
if (windowWidth > 991) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Keelando any idea if .width() can ever return null?

@alexnguyenn alexnguyenn merged commit 3335f3b into main Dec 5, 2021
@alexnguyenn alexnguyenn deleted the apply-eslint-prettier branch December 5, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants