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

Add eslint to react boilerplates. #837

Merged
merged 15 commits into from
May 15, 2023
Merged

Conversation

Abhirup-99
Copy link
Contributor

Fixes #804.

@Abhirup-99 Abhirup-99 force-pushed the add-eslint branch 2 times, most recently from 817974b to 020db0f Compare May 1, 2023 16:59
@brillout
Copy link
Member

brillout commented May 2, 2023

Neat 👌

LGTM. Although I wonder:

  • In Vite's PR, they added settings. Thoughts on this? Is omitting settings equivalent to settings: { react: { version: 'detect' } }?
  • Why omtting plugin:react/jsx-runtime for the TS boilerplate?
  • Can the options of "lint": "eslint src --ext js,jsx --report-unused-disable-directives --max-warnings 0" be set in .eslintrc.cjs? Would be neat to only have "lint": "eslint".

Good idea of adding prop-types for the JS boilerplate. FYI I went ahead and made some minor improvements. Notable change:

  • I added npm run lint to the prod npm script. Let me know if you see problems with that. The idea here is to ensure that linting is green when running the CI (of VPS and perspectively the app's CI).

@brillout
Copy link
Member

brillout commented May 8, 2023

@rtritto thoughts on this?

@rtritto
Copy link
Contributor

rtritto commented May 8, 2023

LGTM.

Note: for .eslintrc, I prefer to use the yml extension (more cleaner).

@brillout
Copy link
Member

brillout commented May 8, 2023

👍 And any thoughts about #837 (comment)?

@rtritto
Copy link
Contributor

rtritto commented May 8, 2023

  • In Vite's PR, they added settings. Thoughts on this? Is omitting settings equivalent to settings: { react: { version: 'detect' } }?

Idk if it's equivalent, so it's safe to use detect.

  • Why omtting plugin:react/jsx-runtime for the TS boilerplate?

It needs to be added.

  • Can the options of "lint": "eslint src --ext js,jsx --report-unused-disable-directives --max-warnings 0" be set in .eslintrc.cjs? Would be neat to only have "lint": "eslint".

Yes, script should be changed to "lint": "eslint ." and move the script options to eslintrc file.

@brillout
Copy link
Member

brillout commented May 8, 2023

Yes, script should be changed to "lint": "eslint ." and move the script options to eslintrc file.

👍

@Abhirup-99 Up for finish the PR? Let me know if you're busy and I'll finish it.

@brillout brillout merged commit 9b02373 into vikejs:main May 15, 2023
@brillout
Copy link
Member

Thanks for the PR & comments.

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.

Add ESLint to React boilerplates
3 participants