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

refactor eslint configuration #3253

Merged
merged 3 commits into from
Mar 20, 2022
Merged

refactor eslint configuration #3253

merged 3 commits into from
Mar 20, 2022

Conversation

DukeManh
Copy link
Contributor

@DukeManh DukeManh commented Mar 18, 2022

I try my best to refactor our shared ESlint configuration.

Changes made:

  • Since we publish @senecacdot/eslint-config-telescope as a package, other eslint configure can extend it just like airbnb-config, for example. Other configuration doesn't have to go into overrides
  • Use EcmaVersion 2021
  • Use recommended react-native config
  • Install SuperTest as main dependency instead of dev, following ESlint recommendation
  • Fix Eslint hangs #3245 by changing eslint file pattern from **/* (which might include outside files) to ./**/*
  • Add 'ts,tsx' to file import resolver
  • Use .gitignore as a .eslintignore (so cool)
  • Use '@typescript-eslint/eslint-plugin' in src/web only
  • Use '@eslint-plugin-react-native' in src/mobile only

@gitpod-io
Copy link

gitpod-io bot commented Mar 18, 2022

@DukeManh
Copy link
Contributor Author

The ESLint test is expected to fail because it's still using the old config.

@@ -38,7 +38,8 @@
"react-use": "17.3.2",
"smoothscroll-polyfill": "0.4.4",
"swr": "1.2.2",
"yup": "0.32.11"
"yup": "0.32.11",
"@testing-library/react": "12.1.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit but @ precedes alpha letters

Comment on lines 53 to 54
"@typescript-eslint/eslint-plugin": "4.33.0",
"@typescript-eslint/parser": "4.33.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@RC-Lee RC-Lee requested review from aserputov and JiaHua-Zou March 18, 2022 01:18
@@ -38,7 +38,8 @@
"react-use": "17.3.2",
"smoothscroll-polyfill": "0.4.4",
"swr": "1.2.2",
"yup": "0.32.11"
"yup": "0.32.11",
"@testing-library/react": "12.1.4"
},
"devDependencies": {
"@senecacdot/eslint-config-telescope": "1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

The ESLint is failing but it's expected due to the tools/eslint/index.js changes in this PR

'import/resolver': {
node: {
extensions: ['.js', '.jsx', '.ts', '.tsx'],

This import/resolver will resolve the 155 errors. To pass ESLint locally, change this line to

"@senecacdot/eslint-config-telescope": "workspace:1.0.1",

Copy link
Contributor Author

@DukeManh DukeManh Mar 18, 2022

Choose a reason for hiding this comment

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

Thanks @cindy.
Or you can manually link eslint: cd <subdir> && pnpm link ./<relativepath-to-eslint>

Copy link
Contributor

Choose a reason for hiding this comment

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

Err you tagged angry-raccoon-python-developer-cindy. Also good to know the pnpm link method. Knowing CLI commands makes things shorter :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, those names are always taken

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I don't have an SEO-friendly name. Maybe when I'm married

@DukeManh DukeManh self-assigned this Mar 18, 2022
@DukeManh DukeManh added the developer experience Helping the Developer Experience label Mar 18, 2022
@DukeManh DukeManh added this to the 2.9 Release milestone Mar 18, 2022
cindyorangis
cindyorangis previously approved these changes Mar 18, 2022
@JiaHua-Zou
Copy link
Contributor

JiaHua-Zou commented Mar 18, 2022

I tried rerunning the CI. It looks like the eslint is failing.

@DukeManh
Copy link
Contributor Author

I tried rerunning the CI. It looks like the eslint is failing.

See @cindyledev's comment above

@DukeManh
Copy link
Contributor Author

Rebased.

cindyorangis
cindyorangis previously approved these changes Mar 19, 2022
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

eslint in CI is unhappy

@DukeManh
Copy link
Contributor Author

@humphd, because it's using [email protected], once this goes in we'll release a new version.

@DukeManh DukeManh requested a review from humphd March 19, 2022 20:15
cindyorangis
cindyorangis previously approved these changes Mar 19, 2022
Copy link
Contributor

@menghif menghif left a comment

Choose a reason for hiding this comment

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

You can increase the version of eslint-config-telescope here. I don't think we need to do it in a separate PR

@DukeManh
Copy link
Contributor Author

What's the appropriate version of the package do you think.

@humphd
Copy link
Contributor

humphd commented Mar 19, 2022

1.1.0 of 1.0.2, either is fine.

@DukeManh
Copy link
Contributor Author

Ok I updated the version, I assume it will be automatically published? Then, we can trigger a Renovate bot for bump v1.0.1 -> v1.1.0 in sub-packages.

@humphd
Copy link
Contributor

humphd commented Mar 20, 2022

Correct. Merging on master will update it.

@menghif menghif merged commit 2145b72 into Seneca-CDOT:master Mar 20, 2022
@cindyorangis cindyorangis mentioned this pull request Mar 20, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer experience Helping the Developer Experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eslint hangs
6 participants