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

[4.5.3][4.6.0-20211212] wrong diagnostic on specifier-specific "type" and importsNotUsedAsValues="error" #47118

Closed
AviVahl opened this issue Dec 12, 2021 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@AviVahl
Copy link

AviVahl commented Dec 12, 2021

Bug Report

πŸ”Ž Search Terms

importsNotUsedAsValues

πŸ•— Version

4.5.3, when specifier-specific type keyword was added.

⏯ Playground Link

The playground doesn't allow setting importsNotUsedAsValues

πŸ’» Code

// this works

import type { VFC } from 'react';
export const Hello: VFC<{}> = () => <main>Hello World</main>;

and:

// this reports diagnostics

import { type VFC } from 'react';
export const Hello: VFC<{}> = () => <main>Hello World</main>;

should behave the same (no error)

πŸ™ Actual behavior

This import is never used as a value and must use 'import type' because 'importsNotUsedAsValues' is set to 'error'.ts(1371) on second piece of code.

πŸ™‚ Expected behavior

Both examples should not report diagnostics.

@andrewbranch I feel like this is your cup of tea. :)

@AviVahl AviVahl changed the title [4.5.2][4.6.0-20211212] wrong diagnostic on specifier-specific "type" and importsNotUsedAsValues="error" [4.5.3][4.6.0-20211212] wrong diagnostic on specifier-specific "type" and importsNotUsedAsValues="error" Dec 12, 2021
@MartinJohns
Copy link
Contributor

I have never seen this syntax. Where did you get it from? All documentation I can find always uses import type, which then counts for all imported types from this statement. I don't think the type is a modifier for individual imports. I would actually expect this to be an error.

@AviVahl
Copy link
Author

AviVahl commented Dec 12, 2021

@MartinJohns it was added in 4.5: https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#type-on-import-names

@MartinJohns
Copy link
Contributor

Thank you, I definitely missed that one! Then I absolutely agree with your expected behaviour.

@andrewbranch
Copy link
Member

I think this is working as intended? When you use --importsNotUsedAsValues error, you are saying β€œIf I write an import declaration, (a) always emit it to JS, and (b) make sure there is an apparent reason why I wrote it in such a way that it needs to be in the JS.” When you write import { type VFC } from "react", you are asking for import {} from "react" to be emitted to the JS. That request doesn’t pass muster with --importsNotUsedAsValues error. It sees an import that is going to be emitted to JS needlessly.

The logic goes like this:

  • import type { VFC } from "react" emits nothing (because what could it emit?)
  • import { type VFC } from "react" emits import {} from "react"
    • This subsequently gets removed under --importsNotUsedAsValues remove (the default) because it is an import that doesn't bring in anything that gets used in an emitting position. (In actuality, these two steps are just one swoop by the transformer, but I think it makes more sense to think of it as two.)

Does that clear it up?

@andrewbranch
Copy link
Member

Another helpful piece of the mental model:

  • import type operates at the ImportDeclaration level
  • import { type ... } operates at the ImportSpecifier level
  • importsNotUsedAsValues operates at the ImportDeclaration level (this is the one that’s easy to mix up)

@Pika-Pool
Copy link

...but it would be nice to avoid two import statements for the same module. That’s part of why TypeScript 4.5 allows a type modifier on individual named imports, so that you can mix and match as needed.
-- taken from https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#type-on-import-names

I still have to write 2 import statements if I have this flag set to error. I think this should be fixed

@AviVahl
Copy link
Author

AviVahl commented Dec 14, 2021

There are several oddities when working with the model you're describing, @andrewbranch
Consider the following code:

import { memo, type VFC, type HTMLAttributes } from 'react';

export const Main: VFC<HTMLAttributes<HTMLElement>> = memo((props) => <main {...props}>Hello World</main>);

If I remove usage and import specifier of memo, I must also remove the type keyword from the other two specifiers and re-add it as prefix of the import declaration.

I guess my (wrong) assumption was:
"If all specifiers of an import declaration are prefixed with type, it's the same as prefixing the entire import declaration with type"

I understand it's the same as to how the specifiers themselves behave in the module, but it's not the same to whether the import declaration is removed or not.

I won't lie, this is not the most intuitive behavior.

@andrewbranch
Copy link
Member

"If all specifiers of an import declaration are prefixed with type, it's the same as prefixing the entire import declaration with type"

This is true under --importsNotUsedAsValues remove (the default). It sounds like that’s what you want.

@harunurhan
Copy link
Member

@andrewbranch I don't think this was really clear in release notes, it sounded like just a syntatic sugar so we won't need another import statement. I thought if all members are type, then TSC would drop the entire import statement.

We can get this behaviour with --importsNotUsedAsValues remove, but is it possible to get it on TS v5+ using --verbatimModuleSyntax?

Considering the eslint rule people use for enforcing and auto fixing type imports also introduces this option as fixStyle without any documentation that it would change what TSC would emit, I think many people will fall into this trap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants