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

feat(unstable): enable importsNotUsedAsValues #7413

Merged

Conversation

lucacasonato
Copy link
Member

It seems that "isolatedModules": true does not catch all errors that can occur with a --nocheck emit. It looks like "importsNotUsedAsValues": "error" is also needed because isolatedModules does not catch import { MyType } from "./file.ts";, only type re-exports.

cc @kitsonk

@bartlomieju bartlomieju added this to the 1.4.0 milestone Sep 10, 2020
@bartlomieju
Copy link
Member

This PR is a blocker for 1.4.0

@nayeemrmn
Copy link
Collaborator

Theoretically we shouldn't need importsNotUsedAsValues because such imports can just be stripped either way, right? Otherwise this case would be covered by isolatedModules.

We need this now, but it should be a milestone for swc / swc integration to only require isolatedModules.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 10, 2020

@lucacasonato do you have a repro of something that works with "isolatedModules": true but fails under --no-check? I would like to understand what swc might be missing, because it should elide imports that do not have a runtime code position.

I agree we should turn it on though.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 10, 2020

Ah, found the problem. SWC does omit imports not used as values. However it incorrectly requires the import to be used at least once as a type to do this. So this is an SWC bug, and it only occurs in the edge case of completely unused imports. No hurry to address it with this option.

cc @kdy1

Off-topic (Another problem: `import { usedAsTypeOnly } from "./foo.ts"` should be stripped down to `import "./foo.ts"`, but is incorrectly removed outright. EDIT: Occurs in tsc as well.)

@kdy1
Copy link

kdy1 commented Sep 11, 2020

It's not a bug as typescript actually does same thing, and that's why type-only imports are introduced.
Is unused import stripped out?

@nayeemrmn
Copy link
Collaborator

@kdy1 A completely unused import is not stripped by swc, but is by tsc.

@kdy1
Copy link

kdy1 commented Sep 11, 2020

Can you check if it works with swc-project/swc#1060 ? Branch lives at https://github.com/kdy1/swc/tree/ts-strip

@kitsonk
Copy link
Contributor

kitsonk commented Sep 11, 2020

The only thing tsc will preserve is a bare import: import "foo";. If everything other imports are unused in the emit, it will always elide the import. I know it has led to "confusion" over the years of that behaviour, where people are expecting modules to be loaded for their side-effects that aren't.

@nayeemrmn

This comment has been minimized.

@nayeemrmn

This comment has been minimized.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, I agree with @lucacasonato that we should use this setting at least temporarily to give users more descriptive errors.

@lucacasonato lucacasonato merged commit fbb18d4 into denoland:master Sep 11, 2020
@lucacasonato lucacasonato deleted the imports_not_used_as_values branch September 11, 2020 14:22
@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 11, 2020

@bartlomieju So do we expect this to hit stable before swc-project/swc#1060 lands? As I said, this bug only occurs in the edge case that the user has mistakenly left a completely unused type import. The restrictions it introduces are more than that.

The issue with side-effect imports not being left behind (#7413 (comment)) is unrelated.

@bartlomieju
Copy link
Member

@nayeemrmn no, this setting will be default and ignored from tsconfig.json in 1.5.0 in the earliest.

@ynwd
Copy link

ynwd commented Sep 25, 2020

Run deno run --watch --unstable -A main.ts.

And get error:

error: TS1371 [ERROR]: This import is never used as a value and must use 'import type' because the 'importsNotUsedAsValues' is set to 'error'.

There is no error when run deno run -A main.ts

@lucacasonato
Copy link
Member Author

Yes - this is intentional. See https://deno.land/posts/v1.4#stricter-type-checks-in-code--unstablecode

luke-john added a commit to luke-john/yargs-parser that referenced this pull request Oct 1, 2020
In version 1.4 Deno adopted;

- the tsconfig setting [importsNotUsedAsValues](https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues) - denoland/deno#7413

This requires the use of;
- type only imports for values which are only used as types (`importsNotUsedAsValues`)
luke-john added a commit to luke-john/y18n that referenced this pull request Oct 1, 2020
In version 1.4 Deno adopted;

- the tsconfig setting [importsNotUsedAsValues](https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues) - denoland/deno#7413

This requires the use of;
- type only imports for values which are only used as types (`importsNotUsedAsValues`)
bcoe pushed a commit to yargs/yargs-parser that referenced this pull request Oct 1, 2020
In version 1.4 Deno adopted;

- the tsconfig setting [importsNotUsedAsValues](https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues) - denoland/deno#7413

This requires the use of;
- type only imports for values which are only used as types (`importsNotUsedAsValues`)
bcoe pushed a commit to yargs/y18n that referenced this pull request Oct 1, 2020
In version 1.4 Deno adopted;

- the tsconfig setting [importsNotUsedAsValues](https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues) - denoland/deno#7413

This requires the use of;
- type only imports for values which are only used as types (`importsNotUsedAsValues`)
bcoe added a commit to yargs/yargs that referenced this pull request Oct 1, 2020
In version 1.4 Deno adopted;

- the tsconfig setting [importsNotUsedAsValues](https://www.typescriptlang.org/tsconfig#importsNotUsedAsValues) - denoland/deno#7413
- the tsconfig setting [isolatedModules](https://www.typescriptlang.org/tsconfig#isolatedModules) when using the deno `--unstable` flag - denoland/deno#7326 ([intended to be enabled by default in 1.5](denoland/deno#7326))

These require the use of;
- type only imports for values which are only used as types (`importsNotUsedAsValues`)
- type only exports for types which are re-exported (`isolatedModules`)

Co-authored-by: Benjamin E. Coe <bencoe@google.com>
nayeemrmn added a commit to nayeemrmn/deno that referenced this pull request Oct 2, 2020
lucacasonato pushed a commit that referenced this pull request Oct 3, 2020
JavascriptMick pushed a commit to JavascriptMick/deno that referenced this pull request Oct 5, 2020
Soremwar pushed a commit to Soremwar/deno that referenced this pull request Oct 6, 2020
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 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

6 participants