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: add overrides from eslint-recommended also for .vue files #40

Merged
merged 2 commits into from
Jun 9, 2022
Merged

fix: add overrides from eslint-recommended also for .vue files #40

merged 2 commits into from
Jun 9, 2022

Conversation

DrJume
Copy link
Contributor

@DrJume DrJume commented Mar 2, 2022

No description provided.

@haoqunjiang haoqunjiang merged commit d27a71a into vuejs:main Jun 9, 2022
@haoqunjiang
Copy link
Member

🤔 This breaks the allow-js test case:


(should fail on no-const-assign, but was overridden)

I've thought about adding a dedicated allow-js ruleset, but after reading the actual rules, I think it would be easier to revert this PR.

  • Most of the overriding rules are cases that TS already covers, but warning about them again in ESLint won't hurt;
  • As for the additional rules that are turned on: no-var, prefer-const, prefer-rest-params, prefer-spread, we can add them separately in this package, rather than inheriting from the upstream config.

@DrJume
Copy link
Contributor Author

DrJume commented Jun 10, 2022

Yes, I can see your point.
The reason for this PR was, because I didn't like the double errors.
I also think to remember that some of the ESLint warnings did not behave well with TS, creating false positives, but I am not sure I remember this correctly.

It would be awesome if we could match the .vue file but with a lang: [ts, tsx] parameter. Would this be possible with the vue-eslint-parser?

@haoqunjiang
Copy link
Member

As far as I know, it's not possible due to limitations of eslint-plugin-vue (and maybe ESLint too) vuejs/eslint-plugin-vue#1523 (comment)

@haoqunjiang
Copy link
Member

I also think to remember that some of the ESLint warnings did not behave well with TS, creating false positives, but I am not sure I remember this correctly.

Yeah, there are, but not the ones in the eslint-recommended ruleset.

It's in recommended, and when such a rule gets disabled, a corresponding typescript-eslint rule will be enabled:
https://github.com/typescript-eslint/typescript-eslint/blob/dc1f9309cf04aa7314e758980ac687558482f47f/packages/eslint-plugin/src/configs/recommended.ts#L11-L12

haoqunjiang added a commit that referenced this pull request Jun 10, 2022
See the reasoning at #40 (comment)
@DrJume
Copy link
Contributor Author

DrJume commented Jun 10, 2022

I think that if you use @vue/eslint-config-typescript every <script> should have lang="ts". Because mixing js and ts is not best practise.
I would try to make a opt-out config, only if you mix js and ts.

@haoqunjiang
Copy link
Member

haoqunjiang commented Jun 10, 2022

I thought about offering an opt-out config too. But now I think it would require a more convincing motivation than the double-error issue.

There will always be projects with allowJs turned on (most likely are projects in the transition from JS to TS).
It'll be too troublesome to offer them yet another choice with only subtle differences, yet we have to document these, which, many might not even read.

So, I don't want to bring in more maintenance burden and make newcomers harder to get into the ecosystem, unless there's absolute necessity.

robertstewart89 added a commit to robertstewart89/Eslint-config-develeop-laravel that referenced this pull request Sep 24, 2022
goldentroll added a commit to goldentroll/eslint-config-typescript that referenced this pull request Mar 14, 2023
@DrJume DrJume deleted the patch-1 branch June 1, 2023 18:38
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.

2 participants