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

feat: flat config, eslint plugin, and nuxt module #332

Merged
merged 22 commits into from
Mar 19, 2024
Merged

feat: flat config, eslint plugin, and nuxt module #332

merged 22 commits into from
Mar 19, 2024

Conversation

antfu
Copy link
Member

@antfu antfu commented Mar 11, 2024

This PR merges the experiments in https://github.com/antfu/nuxt-module-eslint-config and made changes:

  • @nuxt/eslint-config
    • Now exposes flat config sub-entry @nuxt/eslint-config/flat
  • @nuxt/eslint-plugin
    • New package, with just one rule for process.env.dev atm. We could add more later
  • @nuxt/eslint
    • New Nuxt module, that generates .nuxt/eslint.config.mjs based on project structure and user configuration.

After this PR, we could rename the repo to nuxt/eslint, as the central place for hosting eslint related Nuxt integrations

Issues

Features

Docs

Upgrades

Open Questions

  • Q1. Should we introduce a standard docs site for all these packages? We could have an FAQ page to explain the difference between all these packages and the legacy ones.
  • Q2. What should we do with https://github.com/nuxt-modules/eslint? I personally would be against running eslint along with the dev server (the idea is similar to not running type check in dev). The inline ESLint configuration for the module might also be misaligned with the configuration run in the CLI, which could cause confusion. eslint.config.js should be the single source of truth IMO. I would suggest we explain its downside in the README and soft-deprecate that module.

Copy link
Member

atinux commented Mar 11, 2024

Amazing work 🚀

Question 1: do you plan to write a docs site like https://eslint.vuejs.org/ ?

Of adding more content in https://nuxt.com/docs/guide/concepts/code-style could do the work?

Q2: 100% agree with you, you have my go.

@antfu antfu requested a review from danielroe March 12, 2024 00:36
@antfu
Copy link
Member Author

antfu commented Mar 12, 2024

I won't mind it being eslint.nuxt.com or a section in nuxt.com - it's just that we need structural docs with a bit rich interactivity (good to have) to present different packages/concepts/rules in a nice way. If being nuxt.com, I guess we could configure another remote source so that the docs can still co-located inside this repo?

About Q2, I asked on Twitter, some comments for reference: https://twitter.com/antfu7/status/1767279700148174971

/cc @ricardogobbosouza in case you might have more insights about it. I personally wouldn't mind to have the module for ppl who need that capability (quicker feedback, IDE agnostic etc.), but I feel it shouldn't be the default eslint module. If want, we could also merge it into this repo, but with a different, more explicit name to indicate its approach. What do you think?

@ricardogobbosouza
Copy link
Contributor

ricardogobbosouza commented Mar 12, 2024

Hi @antfu

Q1: I believe that a website eslint.nuxt.com is the best option and explains each package and module.
I would also put everything in a single nuxt/eslint repository

Q2: The module https://github.com/nuxt-modules/eslint is very useful, especially when we work as a team and not everyone has the IDE configured correctly, as mentioned by https://twitter.com/MatteoRigoni/status/1767466485834715328
I personally prefer to run alongside the development server.

In the nuxt/eslint repository, in my opinion, there would be packages and one for documentation:

  • @nuxt/eslint-config
    • Now exposes flat config sub-entry @nuxt/eslint-config/flat
  • @nuxt/eslint-plugin
    • New package, with just one rule for process.env.dev atm. We could add more later
  • @nuxt/eslint
    • New Nuxt module, that generates .nuxt/eslint.config.mjs based on project structure and user configuration
  • @nuxt/eslint-moule
    • The module that runs eslint on the development server

@antfu
Copy link
Member Author

antfu commented Mar 15, 2024

@ricardogobbosouza Thanks for the insights! Now I see the situation better, and yes, I think the dev server check makes sense in those cases. Tho I still think in a way it shouldn't be the default option, especially for beginners, and also I am a bit worried about the module name @nuxt/eslint and @nuxtjs/eslint-module would be confusing and not explicit enough.

What do you think about the idea that we join the force merge them together as a single module, and we could have an explicit configuration like:

export default defineNuxtConfig({
  modules: ['@nuxt/eslint'],
  eslint: {
    checker: true // or an options object 
  }
})

(ofc, a better name is needed, I suppose it also blocks the build process so I didn't use devChecker)

So that we could even have a build time only checker only if ppl want:

export default defineNuxtConfig({
  modules: ['@nuxt/eslint'],
  eslint: {
    checker: {
      dev: false,
      build: true
    }
  }
})

WDYT?

@ricardogobbosouza
Copy link
Contributor

I agree, in merging into a single module

@ricardogobbosouza Thanks for the insights! Now I see the situation better, and yes, I think the dev server check makes sense in those cases. Tho I still think in a way it shouldn't be the default option, especially for beginners, and also I am a bit worried about the module name @nuxt/eslint and @nuxtjs/eslint-module would be confusing and not explicit enough.

What do you think about the idea that we join the force merge them together as a single module, and we could have an explicit configuration like:

export default defineNuxtConfig({
  modules: ['@nuxt/eslint'],
  eslint: {
    checker: true // or an options object 
  }
})

(ofc, a better name is needed, I suppose it also blocks the build process so I didn't use devChecker)

So that we could even have a build time only checker only if ppl want:

export default defineNuxtConfig({
  modules: ['@nuxt/eslint'],
  eslint: {
    checker: {
      dev: false,
      build: true
    }
  }
})

WDYT?

I agree to merge them together as a single module

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Phenomenal work @antfu. I'm really happy with this direction - this will save all of us so much time and effort.

I'm sure you don't need permission, but do feel free to merge and iterate at will. ❤️

antfu and others added 6 commits March 18, 2024 16:43
@antfu antfu marked this pull request as ready for review March 18, 2024 17:02
@antfu
Copy link
Member Author

antfu commented Mar 18, 2024

Ok, this should be good to go. Tasks after this gets merged:

/cc @ricardogobbosouza If you got a chance to take a look, I have migrated the implementation of https://github.com/nuxt-modules/eslint. This is under an opt-in checker option. I also added some basic docs and migration guides - let me know what you think. I also took the nuxt-modules/eslint#124 from @ModyQyW to support flat config btw.

/cc @DamianGlowala If you have any comments

Copy link
Contributor

@ricardogobbosouza ricardogobbosouza left a comment

Choose a reason for hiding this comment

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

Nice @antfu
It's a good start to improving the user experience with eslint

  • Need to add support for flat config in webpack
if (isUsingFlatConfig) {
  options.configType = 'flat'
  options.eslintPath = 'eslint/use-at-your-own-risk'
  options.overrideConfigFile: flatConfigFiles.filter(file => existsSync(resolve(nuxt.options.rootDir, file)))[0]
}

packages/module/src/modules/checker.ts Outdated Show resolved Hide resolved
packages/module/src/modules/checker.ts Show resolved Hide resolved
packages/module/src/modules/checker.ts Outdated Show resolved Hide resolved
@ricardogobbosouza
Copy link
Contributor

I can help you with some things, if you think it's necessary.

  • I will update https://github.com/nuxt-modules/eslint
  • Respond to some open issues.
  • Continue to support @nuxtjs/eslint-module until we both feel that @nuxt/eslint is mature enough to be a straightforward, bug-free migration (perhaps we could indicate this with a v1 release?)

@antfu
Copy link
Member Author

antfu commented Mar 18, 2024

@ricardogobbosouza Yeah, I think it makes sense to keep supporting both for a while. For the next step, we will do some alpha releases to make sure everything works as expected.

@leosvelperez
Copy link

leosvelperez commented Mar 19, 2024

@antfu, thanks for this! Could you consider splitting the change to update the typescript-eslint/* packages into a separate PR that could be released quicker than all the changes in this PR? This PR is more significant in scope and would take longer to release in a stable version. Extracting the upgrade of the typescript-eslint/* packages would allow for addressing the associated issues more quickly.

@antfu
Copy link
Member Author

antfu commented Mar 19, 2024

This PR will be merged soon, the basic eslintrc remains unchanged just with deps upgrades - you should be able to upgrade directly

@leosvelperez
Copy link

Will a stable version be released soon? From the conversation I thought it would need some alpha/beta versions first.

@antfu
Copy link
Member Author

antfu commented Mar 19, 2024

I suppose it would take around a week - but you can use beta version if you are hurry to upgrade typescript-eslint

@leosvelperez
Copy link

For context, I'm updating Nx to support TS 5.4 (needed for Angular 17.3), TypeScript ESLint ^7.2.0, but it currently conflicts with our Nuxt support. We try to avoid generating projects using pre-release versions, so batching the TypeScript ESLint change (which seems a small change) together with a bigger feature set prevents using a stable version for other tooling.

I'd be happy to send a separate PR with just that upgrade if you agree to split it. Otherwise, we're blocked or have to generate user projects with a pre-release version.

@antfu
Copy link
Member Author

antfu commented Mar 19, 2024

I understand the situation - but you know, you can always use the package manager's resolutions to force an upgrade of your dependencies locally, use our beta release (which doesn't change anything for you), or wait a week for stable, or do local patch - there are multiple workarounds you can do. We as open-source projects, couldn't really release every different combination for every moving part to make it work for every situation - keeping everything the latest is the most reasonable path we could take.

As this discussion is going off-topic, if you want to continue, I'd suggest you to create a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment