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

Allow inlined-value usage of type-only const enums #46556

Closed
wants to merge 2 commits into from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Oct 27, 2021

Allow, e.g.

// @filename value-usage-of-type-only-const-enum.ts
import type { Direction } from "./direction";

const direction = Direction.Up;

if isolatedModules is false.

Motivation

eslint-plugin-import checks whether import statements are resolvable, so wants to know whether an import statement will be elided (by TypeScript) or not. Even if it could predict which imports were candidates for elision, that behavior is configurable (importsNotUsedAsValues), so the plugin takes the conservative view that all imports except type-only imports might or might not be elided.

This is a problem if type-only const enums can't be used as values:

  • User declares export const enum Direction { Up, Down, Left, Right } in direction.d.ts
  • If they don't type-only import { Direction } from "./direction", eslint-plugin-import warns that the import might not be resolvable (at runtime, assuming there's no direction.js to match direction.d.ts)
  • If they do type-only import it (it's a .d.ts without a .js, after all), TypeScript complains that it can't be used as a value (it's a type-only import, after all) import/no-unresolved errors for .d.ts files since 2.25 import-js/eslint-plugin-import#2267 (comment)

The same problem as #40344.

I understand that type-only means type-only, and a type-only import on the right-hand side of an assignment looks really unexpected: #36003 (comment)

On the other hand going back to the motivation for type-only imports, my impression is that the whole deal was predictable import elision? #35200

If isolatedModules is false:

  • Const enum values are compile-time only
  • Whether or not they're used as values, const enum imports are elided (good, because they can be declared in .d.ts -- what's there to import at runtime, if there's no .js?)

For these reasons, I'd like to allow value usage of type-only const enums if isolatedModules is false.

Before

isolatedModules false:

$ tsc value-usage-of-type-only-const-enum.ts
value-usage-of-type-only-const-enum.ts:3:19 - error TS1361: 'Direction' cannot be used as a value because it was imported using 'import type'.

3 const direction = Direction.Up;
                    ~~~~~~~~~

  value-usage-of-type-only-const-enum.ts:1:15
    1 import type { Direction } from "./direction";
                    ~~~~~~~~~
    'Direction' was imported here.


Found 1 error.

isolatedModules true:

$ tsc --isolatedModules value-usage-of-type-only-const-enum.ts
value-usage-of-type-only-const-enum.ts:3:19 - error TS1361: 'Direction' cannot be used as a value because it was imported using 'import type'.

3 const direction = Direction.Up;
                    ~~~~~~~~~

  value-usage-of-type-only-const-enum.ts:1:15
    1 import type { Direction } from "./direction";
                    ~~~~~~~~~
    'Direction' was imported here.

value-usage-of-type-only-const-enum.ts:3:19 - error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

3 const direction = Direction.Up;
                    ~~~~~~~~~


Found 2 errors.

After

isolatedModules false (error no more):

$ tsc value-usage-of-type-only-const-enum.ts

isolatedModules true (same as before):

$ tsc --isolatedModules value-usage-of-type-only-const-enum.ts
value-usage-of-type-only-const-enum.ts:3:19 - error TS1361: 'Direction' cannot be used as a value because it was imported using 'import type'.

3 const direction = Direction.Up;
                    ~~~~~~~~~

  value-usage-of-type-only-const-enum.ts:1:15
    1 import type { Direction } from "./direction";
                    ~~~~~~~~~
    'Direction' was imported here.

value-usage-of-type-only-const-enum.ts:3:19 - error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

3 const direction = Direction.Up;
                    ~~~~~~~~~


Found 2 errors.

Fixes #40344

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 27, 2021
@typescript-bot
Copy link
Collaborator

The TypeScript team hasn't accepted the linked issue #40344. If you can get it accepted, this PR will have a better chance of being reviewed.

@jablko
Copy link
Contributor Author

jablko commented Oct 27, 2021

If direction.d.ts is instead renamed -> direction.ts:

Before

isolatedModules false (same as direction.d.ts):

$ tsc value-usage-of-type-only-const-enum.ts
value-usage-of-type-only-const-enum.ts:3:19 - error TS1361: 'Direction' cannot be used as a value because it was imported using 'import type'.

3 const direction = Direction.Up;
                    ~~~~~~~~~

  value-usage-of-type-only-const-enum.ts:1:15
    1 import type { Direction } from "./direction";
                    ~~~~~~~~~
    'Direction' was imported here.


Found 1 error.

isolatedModules true (same as false):

$ tsc --isolatedModules value-usage-of-type-only-const-enum.ts
value-usage-of-type-only-const-enum.ts:3:19 - error TS1361: 'Direction' cannot be used as a value because it was imported using 'import type'.

3 const direction = Direction.Up;
                    ~~~~~~~~~

  value-usage-of-type-only-const-enum.ts:1:15
    1 import type { Direction } from "./direction";
                    ~~~~~~~~~
    'Direction' was imported here.


Found 1 error.

After

isolatedModules false (error no more):

$ tsc value-usage-of-type-only-const-enum.ts

isolatedModules true (same as before):

$ tsc --isolatedModules value-usage-of-type-only-const-enum.ts
value-usage-of-type-only-const-enum.ts:3:19 - error TS1361: 'Direction' cannot be used as a value because it was imported using 'import type'.

3 const direction = Direction.Up;
                    ~~~~~~~~~

  value-usage-of-type-only-const-enum.ts:1:15
    1 import type { Direction } from "./direction";
                    ~~~~~~~~~
    'Direction' was imported here.


Found 1 error.

@sandersn
Copy link
Member

@andrewbranch this PR is pretty small. From the issue discussion, it sounds like you'd be interested in a fix if it was simple. What do you think about this?

@sandersn sandersn requested review from RyanCavanaugh and removed request for rbuckton November 17, 2021 23:32
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Nov 17, 2021
@andrewbranch
Copy link
Member

We’ve always known this change would be dead simple, but the issue is “Awaiting More Feedback.” The fairly low amount of feedback we’ve received on it has been positive, and I’ve been cautiously open to this change from the beginning. I think the downside is just adding to the confusion/complexity around what import type and isolatedModules each mean. I think we possibly need to have a broader conversation about the interaction between isolatedModules and const enums now that esbuild can inline them. If that becomes the norm for transpiling/minifying TS, then isolatedModules may be the wrong flag to toggle this behavior. At any rate, I don’t think I can unilaterally approve this PR.

@jablko jablko changed the title Allow value usage of type-only const enums if isolatedModules is false Allow inlined-value usage of type-only const enums Feb 3, 2022
@jablko
Copy link
Contributor Author

jablko commented Feb 3, 2022

I'm still stuck on, you e.g. import { AcBrowseToObjectType } from "activex-access" (pretend it's a module) with either importsNotUsedAsValues: "preserve" or preserveValueImports: true:

  • TS can't elide the import or named import because the runtime .js might have side effects (importsNotUsedAsValues) or you might subsequently have eval("console.log(AcBrowseToObjectType.acBrowseToForm)") (preserveValueImports),
  • but preserving the import/named import is problematic because there might not be any runtime .js or runtime named export.
  • Currently, if you explicitly elide the import (import type { AcBrowseToObjectType }) or named import (import { type AcBrowseToObjectType }) you get an error (which this PR suppresses) if you try to use its (inlined) value (unless you eval("AcBrowseToObjectType.acBrowseToForm") ... or // @ts-expect-error).

On the other hand I'm cool waiting for more feedback 👍, in case a better solution can be found, or you can sidestep it entirely, or I'm missing the bigger picture, or there are developments on the horizon ...

If that becomes the norm for transpiling/minifying TS, then isolatedModules may be the wrong flag to toggle this behavior.

Instead of "if isolatedModules is false" I should have said "if inlined", whether by isolatedModules: false or, in future, some other condition. (I've edited the PR title accordingly.)

@sandersn sandersn moved this from Waiting on reviewers to Waiting on author in PR Backlog Feb 5, 2022
@jablko
Copy link
Contributor Author

jablko commented Feb 9, 2022

I realized another way of putting this is making type-only and ambient const enums equivalent (without the same equivalence for non-const enums).

Roughly, making type-only and .d.ts const enums equivalent.

@sandersn
Copy link
Member

To help with PR housekeeping, I'm going to close this PR while it's still waiting on its bug to be accepted.

@sandersn sandersn closed this Mar 15, 2022
PR Backlog automation moved this from Waiting on author to Done Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Allow importing both type entity and value entity for "const enum" through "import type"
4 participants