8000 [consistent-type-imports] ignore imports marked as used · Issue #4508 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

[consistent-type-imports] ignore imports marked as used #450 8000 8

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.

Sign up for GitHub

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

Jump to bottom
Closed
FloEdelmann opened this issue Feb 3, 2022 · 9 comments
Closed

[consistent-type-imports] ignore imports marked as used #4508

FloEdelmann opened this issue Feb 3, 2022 · 9 comments
Labels
external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin vue issues relating to vue support

Comments

@FloEdelmann
Copy link
Contributor

Description

Vue.js 3 has a syntax (<script setup>) that allows importing stuff in the JavaScript/TypeScript section, and using it in the HTML/template section:

<script setup lang="ts">
import { ref } from "vue";
import MyComponent from "./MyComponent.vue";

type ComponentType = typeof MyComponent extends new () => infer T ? T : never;

const myRef = ref<ComponentType>();
</script>

<template>
  <MyComponent ref="myRef" />
</template>

Here, @typescript-eslint/consistent-type-imports would report the MyComponent import, as it thinks it's only being used as a type. However, it is actually also used as a value in the template. So applying the autofix breaks the Vue build.

Vue + typescript-eslint users are likely to also have the eslint-plugin-vue installed. There, we have rules that mark variables and imports from the <script setup> section with ESLint's markVariableAsUsed function when they are used somewhere in the <template> section.

Would it be possible to ignore imports that have been marked with this function in @typescript-eslint/consistent-type-imports?

For reference, this problem has been reported in the eslint-plugin-vue repo first: vuejs/eslint-plugin-vue#1784

Fail

<script setup lang="ts">
import { ref } from "vue";
import MyComponent from "./MyComponent.vue"; // should indeed be a type import

type ComponentType = typeof MyComponent extends new () => infer T ? T : never;

const myRef = ref<ComponentType>();
</script>

<template>
  <div ref="myRef" />
</template>

Pass

<script setup lang="ts">
import { ref } from "vue";
import MyComponent from "./MyComponent.vue"; // should not be a type import, as it's used in the <template>

type ComponentType = typeof MyComponent extends new () => infer T ? T : never;

const myRef = ref<ComponentType>();
</script>

<template>
  <MyComponent ref="myRef" />
</template>
@FloEdelmann FloEdelmann added enhancement: new base rule extension New base rule extension required to handle a TS specific case package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 3, 2022
@FloEdelmann FloEdelmann mentioned this issue Feb 3, 2022
Closed

2 tasks

@bradzacher
Copy link
Member
bradzacher commented Feb 3, 2022 •
edited
Loading

I don't think that operating based on eslintUsed is a good idea because it could mean anything.
Literally any plugin that you have installed could set that flag to true. Meaning that there is a huge can of worms being opened. This would cause false negatives which would be really, really hard to debug.

Ideally the vue parser should augment the scope with the additional information about the usages, rather than just marking the variable as used.

As an example - our scope analyser adds usages for React when JSX is found by augmenting the analysis, even though there isn't an actual React usage in the code.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party vue issues relating to vue support and removed triage Waiting for team members to take a look enhancement: new base rule extension New base rule extension required to handle a TS specific case labels Feb 3, 2022
@FloEdelmann
Copy link
Contributor Author

@ota-meshi Maybe vue-eslint-parser could do something like that? I don't know much about that package.

@ota-meshi
Copy link
Contributor

I'll explain why later, but the conclusion is that it's very difficult to use ScopeManager in vue-eslint-parser to mark template variables.

@bradzacher
Copy link
Member

I'll explain why later, but the conclusion is that it's very difficult to use ScopeManager in vue-eslint-parser to mark template variables.

You already get the variables to mark them as used - couldn't you just synthesise and add references to the imports instead?

@ota-meshi
Copy link
Contributor

vue-eslint-parser parses <template> and <script> as separate ASTs.
ESLint's core rules and other plugin rules only check <script>. This means that popular plugins that don't know the <template>'s own AST will work. For example, the <template> AST does not have an esTreeNodeToTSNodeMap, so trying to check the <template> AST with typescript-eslint's rules can cause a crash.
Since eslint-plugin-vue rules implements to check <template> in addition to that, <template> checking also works.

The ScopeManager only has the scope of <script> because the ASTs of <template> and <script> are separated. This is why it is difficult to mark variables in <template> in ScopeManager.
If we try to mark a variable in <template>, the ScopeManager will contain an Identifier that shouldn't be accessible, which may confuse some rules.

@ota-meshi
Copy link
Contributor
ota-meshi commented Feb 5, 2022 •
edited
Loading

I don't think that operating based on eslintUsed is a good idea because it could mean anything.

I don't think it's a good idea as you say. However, I think it might be a good solution for Vue users as a compromise.
The ESLint core prefer-const rule also seems to use eslintUsed.
https://github.com/eslint/eslint/blob/417191dff0dbfa353675c409e25f27f578ee1559/lib/rules/prefer-const.js#L174

@bradzacher
Copy link
Member
bradzacher commented Feb 5, 2022 •
edited
Loading

However, I think it might be a good solution for Vue users as a compromise.

I don't think we should subject all TS users to a hack just for Vue.
Probably worth making an extension rule within eslint-plugin-vue, if that's the workaround you want to use.

the ScopeManager will contain an Identifier that shouldn't be accessible, which may confuse some rules.

FWIW - as mentioned, we already do some "weird" stuff in our scope manager for references to React from within JSX by adding a reference from itself to itself.

import React from 'react';
//     ^^^^^ Variable.references = [ Reference<Identifier, 'React', {line:1, column: 8}> ]

const x = <div />;

playground with scope tree

Relevant code that does this:

typescript-eslint/packages/scope-manager/src/referencer/Referencer.ts

Lines 122 to 141 in 5ab1d57

private referenceJsxPragma(): void {
if (this.#jsxPragma === null || this.#hasReferencedJsxFactory) {
return;
}
this.#hasReferencedJsxFactory = this.referenceInSomeUpperScope(
this.#jsxPragma,
);
}
private referenceJsxFragment(): void {
if (
this.#jsxFragmentName === null ||
this.#hasReferencedJsxFragmentFactory
) {
return;
}
this.#hasReferencedJsxFragmentFactory = this.referenceInSomeUpperScope(
this.#jsxFragmentName,
);
}

typescript-eslint/packages/scope-manager/src/referencer/Referencer.ts

Lines 103 to 120 in 5ab1d57

/**
* Searches for a variable named "name" in the upper scopes and adds a pseudo-reference from itself to itself
*/
private referenceInSomeUpperScope(name: string): boolean {
let scope = this.scopeManager.currentScope;
while (scope) {
const variable = scope.set.get(name);
if (!variable) {
scope = scope.upper;
continue;
}
scope.referenceValue(variable.identifiers[0]);
return true;
}
return false;
}

This code has been in place for 17 months without issue (#2498).
I don't think there's any reason vue couldn't do the same thing - add a read-only reference from the import to itself instead of marking it as eslintUsed.

@ota-meshi
Copy link
Contributor

FWIW - as mentioned, we already do some "weird" stuff in our scope manager for references to React from within JSX by adding a reference from itself to itself.

Oh. I didn't understand your first comment. Thank you for explaining in detail. I didn't have the idea to add a reference in the same place. I think it's worth trying.

I don't think we should subject all TS users to a hack just for Vue.
Probably worth making an extension rule within eslint-plugin-vue, if that's the workaround you want to use.

Thank you for your opinion. If the method 8000 of using ScopeManager doesn't work, I will consider creating an extension rule in eslint-plugin-vue.

@bradzacher
Copy link
Member

I'll close this here, as you have a tracking issue in your repo.
Feel free to @ me if you need assistance or advice!

@bradzacher bradzacher added external This issue is with another package, not typescript-eslint itself and removed awaiting response Issues waiting for a reply from the OP or another party labels Feb 5, 2022
@ota-meshi ota-meshi mentioned this issue Feb 5, 2022
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin vue issues relating to vue support
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@FloEdelmann @bradzacher @ota-meshi
0