nuxt / create-nuxt-app

Create Nuxt.js App in seconds.
MIT License
3.48k stars 429 forks source link

fix(eslint): revert #682 adding eslint-plugin-vue to devDependency #826

Open scscgit opened 3 years ago

scscgit commented 3 years ago

This reverts commit 7cef1461933464294857ebad31ea0574d2f23e29.

The eslint-plugin-vue isn't configured to be used by the .eslintrc.js, so it's installed redundantly as long as only prettier rule is used.

If officially installed via npx @vue/cli add @vue/cli-plugin-eslint, it adds ['plugin:vue/essential','eslint:recommended','@vue/prettier'] to the extends array; or it can be used with plugin:vue/recommended, but it needs further analysis to avoid some other potential conflicts.

scscgit commented 3 years ago

I see, the eslint-config (rule and npm project both named @nuxtjs/eslint-config-typescript) already includes eslint-plugin-vue as a dependency (both as npm project and as a plugin:vue/recommended rule), so this is just intended to control its version.

Could you link me to any resource regarding the hoisting issue, or did you observe it causing any compilation error that is still relevant? I was doing a project cleanup and didn't notice any issues after removing it, and I'd personally prefer having the version of the transitive dependency kept compatible by eslint-config, so it may be useful for owners of projects based on this generator to have any information about the motivation behind including it explicitly to avoid them making the same mistake. I didn't find any PR related to https://github.com/nuxt/create-nuxt-app/commit/7cef1461933464294857ebad31ea0574d2f23e29. Thanks!

clarkdo commented 3 years ago

Related issue https://github.com/nuxt/create-nuxt-app/issues/682 and https://github.com/nuxt/create-nuxt-app/issues/682

scscgit commented 3 years ago

I wasn't able to reproduce the issue even from that example. I think it's likely that it has been already fixed by some dependency update. Also you linked the same issue twice, did you mean https://github.com/nuxt/eslint-config/issues/134?

clarkdo commented 3 years ago

Sorry https://github.com/nuxt/eslint-config/issues/134 I think the reproduction may need some special env setup like previous npm cache or other deps requires a different version eslint vue. Any specific reason that we have to remove it ?

scscgit commented 3 years ago

In case we keep this here, as a long-term solution I'd suggest to at least list it somewhere (e.g. in Readme) as part of temporary workarounds, so that it's clear to users that it's intended to be removed whenever the project can function without it, rather than making it a permanent choice that we'll always be afraid to remove. (Personally I'd prefer making them opt-in as a separate step, as I'm sure that the number of workarounds will keep growing.)

clarkdo commented 3 years ago

Thanks for all the feedback.

For this specific issue, I think having eslint-plugin-vue in deps is the best approach for now, this is not a generic issue like other transitive deps in real application, create-nuxt-app is a template app for generating apps, so it may have more deps combinations and transitive deps are more complex, if any other package like a UI or nuxt module also depends on eslint-plugin-vue, a wrong version may be installed into root node_modules and cache, so it may lead to unexpected error.

[Off-topic] The motivation is primarily that all of these little workarounds compound,

This is a good point to think about, the purpose of this project is to provide users with a more convenient and easy way to generate nuxt applications, so reducing complexity and improving doc are definitely important. Help and contribution are always welcome, and we all very much appreciate it.

Regarding the linter issues, because @nuxtjs/eslint-config is mainly used by nuxt and modules repos instead of nuxt application repos, I'm wondering if we should remove @nuxtjs/eslint-config from create-nuxt-app and use explicit eslint-plugin-vue and eslint recommended rules, so that the linter configuration and deps can be more consistent and easy to maintain and understand.

cc @nuxt/framework may have other comments.

scscgit commented 3 years ago

Yeah if there are any other motivation like avoiding conflicts of eslint-plugin-vue versions with Nuxt modules (which aren't covered by eslint-config's transitive version), then I'd suggest that it could be made more explicit instead of just having the change be described as "fix(link)" of https://github.com/nuxt/create-nuxt-app/commit/7cef1461933464294857ebad31ea0574d2f23e29, so that users can always find at least one source of any decision (which in ideal scenario wouldn't require even searching through PRs). It makes a great difference to know if something is an exception or a rule, as there's usually no such "free" line of code that we can just ignore. (Right now this wasn't grounded either in any specific exception or any rule, making it difficult to change once it gets obsolete.)

I know that a detailed documentation isn't supposed to be a responsibility of scaffolding project, but it's sadly the only source of truth we have when it comes to compatibility of dependencies. I hope this generator will grow to cover all the major use-cases, and that it'll be the responsibility of third party library maintainers to come and fix any issues here directly rather than hoping that the generator's maintainers will figure out all the peculiar conflicts due to pressure from desperate users, as that's usually already too late 😄 (I think even this PR should be in ideal case an opinionated decision by eslint-plugin-vue or prettier devs, rather than anyone else guessing if it's still needed; especially if they caused the need for a workaround in a first place.)

Regarding replacement of the @nuxtjs/eslint-config || @nuxtjs/eslint-config-typescript, do you mean importing the contents of https://github.com/nuxt/eslint-config/blob/6ac852033430d4c0a9dae17cbfeb9531218d55f3/packages/eslint-config/index.js#L11 directly to the generated project? At a first glance it seems to be an overkill, especially from the standpoint of maintaining updates if the eslint-config makes changes in the future. I've noticed a recent reduction especially from prettier, which now only has 1 rule extension, so this trend seems to be fine if they focus on predicting & resolving the conflicts between different modules, which is probably easiest done if they all provide one primary default. In any case, covering edge cases will be vital, so generator is responsible for that oversight.