solidjs-community / eslint-plugin-solid

Solid-specific linting rules for ESLint.
MIT License
206 stars 24 forks source link

Add basic Flat Config setup to README #96

Closed Mitsunee closed 10 months ago

Mitsunee commented 1 year ago

Adds some basic Flat Config setup instructions.

I tried to keep it as similar as possible to the current configs (with browser+es6 env and using "eslint:recommended" as a base config). I am unsure if impliedStrict is supported in Flat Configs right now, so I omitted that one.

joshwilsonvu commented 1 year ago

Thank you for suggesting this! Just curious, do you happen to have or know of a repository where the flat configuration is working as described here? It would help me with testing this out, though if not I'm happy to test it myself.

Mitsunee commented 1 year ago

I'll set up a quick vite project to clone for testing and post here.

Edit: Here's the repo: https://github.com/Mitsunee/eslint-solid-flatconfig-test
It has the exact config copied from the README in this PR as well as a .vscode/settings.json file to enable experimental support for Flat Config with the ESLint plugin for vscode/codium. I also included two examples for lint rules in comments in App.jsx

Unrelated: I was also going to ask if the setup I currently just have in the readme could be offered as a separate import (i.e. src/flat.ts assuming the types exist already) once it's tested properly. That would make setup for users a lot simpler.

Mitsunee commented 1 year ago

Not sure if you've gotten around to testing my initial proposal yet, but I've been tinkering and researching things a bit more and stumbled upon a promising approach by eslint-plugin-react which seems to allow using the same configs with both the legacy and the newer flat config systems.

I can try apply this same method in this PR (or a new one) if you want, or go with my original idea of adding a separate export such as src/flat.ts (or src/unstable_flat.ts if you prefer) for flat only. Either way I'd love to find a solution as the migration to the new system seems to have reached Stage 3, which includes helping plugin/config authors adopt the new system :)

joshwilsonvu commented 1 year ago

Thanks for looking into this more, and I appreciate your patience as I haven't had much time to work on this. I like the idea of using the same config for both config systems as much as possible—I get the feeling that "legacy" config is still going to be around for years. To make sure I'm understanding right, to do this we would

  1. create a configs directory with the recommended and typescript configs
  2. change the existing src/index.ts file to reference those configs, with a little manipulation to get them into the right shape

I'd love to see this happen—it feels like a better approach to focus on creating good, shareable flat configs, and then porting the legacy configs to use it under the hood.

I can take this if you want, but I'd appreciate your help in making that happen in this PR. Thanks so much!

Mitsunee commented 1 year ago

I agree that it's vital to keep supporting both types of configs for now, especially as ESLint are still at least one major version away from removing support for it and it does take some migration to switch systems.

I'll give this a shot later, just got one question: Setting global variables (previously the "env" key) now uses a separate package which essentially contains a collection of Record<string, true> of all the variables. Would adding this globals package as a dependency be okay, or should I just not include this step in the new flat configs?

Mitsunee commented 1 year ago

made a first draft for the implementation with flat configs. Still needs some more testing, I noticed at least solid/components-return-once did not seem to work in my test project after switching to the config via the package, unsure why. It still works when using the legacy config.

Mitsunee commented 1 year ago

I should actually read errors my IDE shows, the error I did get was not from ESLint but from TypeScript which also complains about duplicate properties.

Turns out the "files" key is important so .jsx are actually checked. For the legacy configs this requires explicitly targeting the files via the --ext flag or path (like the VSCode Extension seems to be doing for legacy configs, but the eslint cli by default does not):

Screenshot_20230610_211642

This behaviour is the same with the current production version of the legacy configs (in fact I was using the one from npm in the screenshot above) so fixing that is not within the scope of this PR (could be done with overrides using the same "files" key I just added to flat configs).

Questions before merging:

joshwilsonvu commented 1 year ago

I'm happy to defer to precedent for all these if there is one, but I understand it's early days.

Mitsunee commented 1 year ago

I also noticed that the typescript config of the current release does not actually include the parserOptions, which I accidentally fixed in this PR.

joshwilsonvu commented 10 months ago

Thank you very much for your patience—ended up matching the flat configs up with the legacy configs for easier migration to flat ESLint. Just wanted to make sure what we recommend is tested and this is what made the testing work. TS users will need to configure the parser for themselves (since there are multiple options out there) so I left parserOptions out.

Thanks again!