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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scscgit
Copy link
Contributor

@scscgit scscgit commented Jul 16, 2021

This reverts commit 7cef146.

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.

This reverts commit 7cef146.

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.
Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

eslint-plugin-vue is used by eslint-config, so adding it in create-nuxt-app is for fixing hoisting dependency issue.

@scscgit
Copy link
Contributor Author

scscgit commented Jul 16, 2021

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 7cef146. Thanks!

@clarkdo
Copy link
Member

clarkdo commented Jul 16, 2021

Related issue #682 and #682

@scscgit
Copy link
Contributor Author

scscgit commented Jul 16, 2021

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 nuxt/eslint#134?

@clarkdo
Copy link
Member

clarkdo commented Jul 17, 2021

Sorry nuxt/eslint#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
Copy link
Contributor Author

scscgit commented Jul 17, 2021

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.)

  • [Off-topic] The motivation is primarily that all of these little workarounds compound, making it exponentially more difficult to maintain, clean, and debug projects, as we have to figure out dozens of similar exceptions from scratch. It's even more difficult to contribute changes if we first have to test any behavior under all combinations (spending hours doubting ourselves, especially if we start with the easiest changes first; when debugging you can't always afford to ignore that something works when it shouldn't). Sadly it has been lately ncreasingly difficult to keep up with the compatibility & recommended combinations of stuff like linters, making us rely on double-checking everything with this generator as a reference; and I'm still just in the process of catching up (causing an increased risk of suggesting incorrect changes just because there's "more than one difference at the same time" if there's no time to test/remember all combinations). I hope one day it'll be reasonable for all developers to set up linters without being punished for it, as it often takes weeks to research their issues (as if Nuxt & Vue didn't already have enough severe issues). That makes me more invested in consistency here, because I could never again recommend anyone who has a more productive use of their time to try to diverge from this generator's defaults...

@clarkdo
Copy link
Member

clarkdo commented Jul 17, 2021

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
Copy link
Contributor Author

scscgit commented Jul 17, 2021

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 7cef146, 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.

@scscgit scscgit changed the title fix(eslint): revert adding eslint-plugin-vue to devDependency fix(eslint): revert #682 fix adding eslint-plugin-vue to devDependency Aug 5, 2021
@scscgit scscgit changed the title fix(eslint): revert #682 fix adding eslint-plugin-vue to devDependency fix(eslint): revert #682 adding eslint-plugin-vue to devDependency Aug 6, 2021
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.

None yet

2 participants