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

chore(eslint): migrate to flat config and simplify #2912

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

sukvvon
Copy link
Contributor

@sukvvon sukvvon commented Dec 18, 2024

Summary

  • Mirgrate legacy .eslintrc to eslint.config.js
  • Simplify from the existing configuration

Check List

  • pnpm run prettier for formatting code and docs

Copy link

vercel bot commented Dec 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
zustand-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 29, 2024 1:50am

Copy link

codesandbox-ci bot commented Dec 18, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

pkg-pr-new bot commented Dec 18, 2024

Open in Stackblitzdemostarter

npm i https://pkg.pr.new/zustand@2912

commit: 4232740

@dai-shi
Copy link
Member

dai-shi commented Dec 18, 2024

This is nice! I wasn't sure when all plugins are ready. They weren't when I last tried.
Are you sure all configs are migrated? No additions or removals?

@sukvvon
Copy link
Contributor Author

sukvvon commented Dec 18, 2024

This is nice! I wasn't sure when all plugins are ready. They weren't when I last tried. Are you sure all configs are migrated? No additions or removals?

@dai-shi i checked all plugins. if it's not i will keep my eyes on this, i migrated with no additions and removals

@dbritto-dev
Copy link
Collaborator

dbritto-dev commented Dec 18, 2024

@dai-shi @sukvvon IMHO, I prefer no config or simple config instead of complex config. In this case I'll keep the current config. We can use something else like OXC lint or Biome and simplify the config.

@dai-shi
Copy link
Member

dai-shi commented Dec 18, 2024

I'll keep the current config.

Do you mean to keep using eslint 8?

@sukvvon
Copy link
Contributor Author

sukvvon commented Dec 18, 2024

@dbritto-dev IMHO, Tools like oxc and biome could be considered as alternatives, but since they are still under development, it might be too early to use them. I believe eslint is still more practical at this point, with abundant references, widespread adoption, and ease of application. For now, as we are currently using eslint, it would be better to focus on supporting eslint and consider simplifying later.

@dbritto-dev
Copy link
Collaborator

dbritto-dev commented Dec 18, 2024

I wouldn't put much effort on eslint is pretty slow and now are better options, apart from that biome is ready for production. This change just make it more complex to maintain and add another dependency to make it work I do not think that worth it.

@dai-shi
Copy link
Member

dai-shi commented Dec 18, 2024

Our ecosystem still depends on eslint plugins. Maybe, eslint-plugin-react-compiler is one of the biggest hurdles. Let's stick with eslint.

@dbritto-dev
Copy link
Collaborator

@dai-shi do we need this change to support react-compiler plugin?

@dbritto-dev
Copy link
Collaborator

I'll keep the current config.

Do you mean to keep using eslint 8?

yeah

@dai-shi
Copy link
Member

dai-shi commented Dec 18, 2024

react-compiler is already included. eslint 8 will no longer be supported. This change is what I wanted to do when eslint 9 was released, but at that point something wasn't ready.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Let's do some practices that I learned with jotaijs/jotai-valtio#11.
The primary goal is to simplify configs and prefer defaults.

  • Use "lint": "eslint ." in package.json and move linter configs to config file
  • Remove eslint-config-prettier package
  • Can we use eslint-import-resolver-typescript package to simplify eslint.config.js?
  • Can we use eslint.config.mjs and use ESM syntax?
  • Let's remove globals packages. The added commend in examples/demo/src/utils/copy-to-clipboard.js seems good.
  • Remove examples/*/eslint.config.* and eslint packages from examples/*/package.json
  • No need to useeslint-plugin-prettier package, as we don't configure stylistic rules.
  • Remove some custom rules that don't cause "errors" in the current zustand/jotai/valtio code. (we keep import order rules.)

@sukvvon
Copy link
Contributor Author

sukvvon commented Dec 23, 2024

Let's do some practices that I learned with jotaijs/jotai-valtio#11. The primary goal is to simplify configs and prefer defaults.

  • Use "lint": "eslint ." in package.json and move linter configs to config file
  • Remove eslint-config-prettier package
  • Can we use eslint-import-resolver-typescript package to simplify eslint.config.js?
  • Can we use eslint.config.mjs and use ESM syntax?
  • Let's remove globals packages. The added commend in examples/demo/src/utils/copy-to-clipboard.js seems good.
  • Remove examples/*/eslint.config.* and eslint packages from examples/*/package.json
  • No need to useeslint-plugin-prettier package, as we don't configure stylistic rules.
  • Remove some custom rules that don't cause "errors" in the current zustand/jotai/valtio code. (we keep import order rules.)

@dai-shi I'll do it step by step 🙋🏻‍♂️

@sukvvon
Copy link
Contributor Author

sukvvon commented Dec 25, 2024

Remove some custom rules that don't cause "errors" in the current zustand/jotai/valtio code. (we keep import order rules.)

@dai-shi I gonna do this ASAP

@sukvvon
Copy link
Contributor Author

sukvvon commented Dec 25, 2024

  • Use "lint": "eslint ." in package.json and move linter configs to config file
  • Remove eslint-config-prettier package
  • Can we use eslint-import-resolver-typescript package to simplify eslint.config.js?
  • Can we use eslint.config.mjs and use ESM syntax?
  • Let's remove globals packages. The added commend in examples/demo/src/utils/copy-to-clipboard.js seems good.
  • Remove examples/*/eslint.config.* and eslint packages from examples/*/package.json
  • No need to use eslint-plugin-prettier package, as we don't configure stylistic rules.
  • Remove some custom rules that don't cause "errors" in the current zustand/jotai/valtio code. (we keep import order rules.)

@dai-shi Done!

eslint.config.mjs Outdated Show resolved Hide resolved
@sukvvon sukvvon requested a review from dai-shi December 25, 2024 14:02
Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Thanks for working on it! Looks much better.

eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Show resolved Hide resolved
@sukvvon sukvvon changed the title chore(eslint): migrate .eslintrc to eslint.config.js chore(eslint): migrate to flat config Dec 29, 2024
@sukvvon sukvvon changed the title chore(eslint): migrate to flat config chore(eslint): migrate to flat config and simplify Dec 29, 2024
@sukvvon sukvvon marked this pull request as ready for review December 29, 2024 06:45
@sukvvon sukvvon requested a review from dai-shi December 29, 2024 06:45
@sukvvon
Copy link
Contributor Author

sukvvon commented Dec 29, 2024

@dai-shi I think we're ready to merge!

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Other than the comment, it looks good to me. Thanks so much for your continued effort!

(I still wonder if we should have eslint for demo, but let's put it aside for this one.)

Can you update Jotai and Valtio PRs too?

Comment on lines -68 to +72
"eslint": "eslint --no-eslintrc --c .eslintrc.json --fix '*.{js,ts}' '{src,tests}/**/*.{ts,tsx}'",
"eslint": "eslint '*.{js,mjs,ts}' '{src,tests}/**/*.{ts,tsx}' --fix ",
"test": "pnpm run '/^test:.*/'",
"test:format": "prettier '*.{js,json,md}' '{examples,src,tests,docs}/**/*.{js,jsx,ts,tsx,md,mdx}' --list-different",
"test:types": "tsc --noEmit",
"test:lint": "eslint --no-eslintrc --c .eslintrc.json '*.{js,ts}' '{src,tests}/**/*.{ts,tsx}'",
"test:lint": "eslint '*.{js,mjs,ts}' '{src,tests}/**/*.{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.

I would like "eslint ." here too, and also for --fix above.

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.

3 participants