microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
160.84k stars 28.21k forks source link

code-layering linter affects `import type` statements #190070

Closed ttrider closed 11 months ago

ttrider commented 11 months ago

In the VS Code codebase we use local/code-layering and local/code-import-patterns to verify validity of import statements.

Currently these linters check both import xxx from 'yyy' and import type xxx from 'yyy' However, importing types is safe in any layer and today it produces linter errors.

I propose to exclude import type statements from local/code-layering and local/code-import-patterns linter rules

mjbvz commented 11 months ago

Type imports are still dependencies even if they aren't runtime errors. Code layering makes sense for them for a code structure perspective

ttrider commented 11 months ago

How about code-import-patterns?

I see at least 11 instances where we have to disable them with

// Importing types is safe in any layer
// eslint-disable-next-line local/code-import-patterns
mjbvz commented 11 months ago

I see at least 11 instances where we have to disable them with

We should either fix those specific lines of code or update the rule for those specific cases (the majority seem to be for xterm-headless)

ttrider commented 11 months ago

I have my PR ready (not submitted yet) to add this check to the linter: https://github.com/microsoft/vscode/blob/6c32fd48c5637a91edd06ffabf8b4f609faa1154/.eslintplugin/utils.ts#L20

        ImportDeclaration: (node: any) => {
            if ((<TSESTree.ImportDeclaration>node).importKind !== 'type') {
                _checkImport((<TSESTree.ImportDeclaration>node).source);
            }
        },

It effectively ignores import type statements.

I agree that ideally, we should fix these imports, but if it is not easy, we may want to consider my fix.

I'm OK with either direction

ttrider commented 11 months ago

I have my PR ready (not submitted yet) to add this check to the linter: https://github.com/microsoft/vscode/blob/6c32fd48c5637a91edd06ffabf8b4f609faa1154/.eslintplugin/utils.ts#L20

        ImportDeclaration: (node: any) => {
            if ((<TSESTree.ImportDeclaration>node).importKind !== 'type') {
                _checkImport((<TSESTree.ImportDeclaration>node).source);
            }
        },

It effectively ignores import type statements.

I agree that ideally, we should fix these imports, but if it is not easy, we may want to consider this fix.

I'm OK with either direction