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

Declare non-const enums if preserveConstEnums is true #46626

Closed
wants to merge 5 commits into from

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Nov 1, 2021

Currently if you tsc --declaration --isolatedModules, the compiler turns non-ambient const enums (.ts files) into ambient const enums (.d.ts files) that can't themselves be consumed with --isolatedModules: Ambient const enums (values -- types are fine) are incompatible with isolatedModules.

In every other way tsc --isolatedModules ignores the const modifier -- treats const enums as non-const:

  • Emits .js values (like preserveConstEnums)
  • Doesn't inline values and doesn't elide imports (unlike preserveConstEnums)

This PR doesn't emit the const modifier if isolatedModules is true, effectively suppressing the const modifier entirely in that case. tsc --declaration --isolatedModules then turns non-ambient const enums into ambient non-const enums which can in turn be consumed with --isolatedModules: #37774 (comment)

The affected scenario is when you:

  1. Produce .d.ts with isolatedModules: true (currently: const modifiers preserved, this PR: const modifiers dropped)
  2. Consume the produced .d.ts:
    • With isolatedModules: true: Currently: error. This PR: no error, values and imports preserved, same as all enums, (non-ambient) const enums included.
    • With isolatedModules: false: Currently: values inlined, imports elided. This PR: same as with isolatedModules: true (values and imports preserved).

When a .d.ts is produced with isolatedModules: true and consumed with isolatedModules: false, what's the point of inlining values and eliding imports? If you consume any part of the produced library that itself uses the const enum values, you'll end up importing the enum's .js values.

So this PR will only introduce imports that don't currently exist if you publish a library that declares const enum values it doesn't itself use, and in that case if you really want to publish isolatedModules-incompatible ambient const enums, produce its .d.ts with isolatedModules: false.

Edit: Inlining enums from other projects comes with pitfalls:

  1. Consumers can inline enum values from upstream version A at compile time and import version B at runtime, with different values, if you're not very careful:

Roughly this PR propagates today's isolatedModules const enum behavior transitively to .d.ts consumers.

Fixes #37774

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

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

factory.createNodeArray(
filter(
node.modifiers,
modifier => !printerOptions.isolatedModules || modifier.kind !== SyntaxKind.ConstKeyword
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively this could be done when parsing vs. emitting const enums?

@jablko
Copy link
Contributor Author

jablko commented Nov 1, 2021

This doesn't fix #37774 if isolatedModules: false, preserveConstEnums: true. The advantage of publishing with isolatedModules: false, preserveConstEnums: true is that you can consume parts of the produced library that themselves use const enum values, without importing the enum's .js values ...

@jablko jablko changed the title Declare non-const enums if isolatedModules is true Declare non-const enums if preserveConstEnums is true Nov 4, 2021
@sandersn
Copy link
Member

@jablko is this a proof of concept? When I skimmed the issue I didn't see that it had arrived at a single solution yet.

@jablko
Copy link
Contributor Author

jablko commented Nov 19, 2021

It's a proposed solution: #37774 (comment), but you're right: It's awaiting feedback (issue and proposal).

I think it's right because:

  1. Publishing ambient const enums is a current problem:
  2. Deconstifying .d.ts files in a build step requires author buyin. The only other mitigation today is to avoid const enums entirely, which has performance implications (though most often small?).
  3. While two out of three ambient const enum problems have proposed remedies (a preserved modifier would address isolatedModules incompatibility, Allow inlined-value usage of type-only const enums #46556 (comment) addresses import elision), there remains a compile time vs. runtime pitfall to inlining inter-project enums (ambient const enums, roughly).

tsc --declaration --preserveConstEnums should automatically not transform const enums -> ambient const enums (deconstify ambient enums), I think.

  • The cases where you want to inline inter-project enums (with consequent pitfall) are already addressed by preserveConstEnums: false.
  • The cases where you want some consumers to inline enums and others not to are:
    1. More confusing than useful?
    2. Doable anyway in a build step, if you know what you're doing (like deconstification is today)?

The difference between this PR and a preserved modifier is that the modifier would presumably inline preserved inter-project const enums if isolatedModules is false. This PR is simpler (no new kind of enum) and desirable in the majority of and the entry-level cases (avoids pitfall, little or no downside?).

@sandersn
Copy link
Member

sandersn commented Dec 7, 2021

Based on the discussion at the December 1st design meeting, we aren't convinced that improving const enums is worthwhile, since people should be using the feature less over time rather than more.

@jablko
Copy link
Contributor Author

jablko commented Dec 8, 2021

All of the const enum pitfalls remain if you don't use preserveConstEnums: true, which should discourage people (as much as today)? 😏

  • Avoiding const enums is preferable, but the number of people who should truly use them isn't zero.
  • In those few cases, the current workaround (a build step) isn't onerous, but this change is commensurately small, and strictly positive, I think?

@sandersn sandersn self-assigned this Feb 19, 2022
@jablko jablko changed the title Declare non-const enums if preserveConstEnums is true Declare plain enums when preserveConstEnums is on Mar 2, 2022
@jablko
Copy link
Contributor Author

jablko commented Mar 2, 2022

I've added a commit to inline plain enums from referenced projects.

Currently you get different behavior consuming .ts files and consuming the .d.ts outputs of those same files: const enums in .ts files are (ordinarily) compatible with --isolatedModules (because it flips on --preserveConstEnums), whereas the .d.ts outputs aren't (ambient const enums are incompatible with single-file transpilation).

This came up again here. An example is you can't use const enums from referenced projects with --isolatedModules. You can use const enums from .ts files, but the CLI uses the .d.ts outputs for referenced projects. That error shows up in the CLI but not in the editor, which defaults to using .ts files.

This PR fixes that, but now you get different behavior between .ts files and their .d.ts outputs when built with --preserveConstEnums and consumed without --isolatedModules: const enums in .ts files are inlined whereas their plain enum .d.ts outputs aren't. The latest commit addresses that.

  • With --isolatedModules, --preserveConstEnums and deconstification solve single-file transpilation compatibility by emitting plain enums. --isolatedModules already treats const enums as plain enums anyway (doesn't inline).
  • Without --isolatedModules it solves other problems with publishing ambient const enums: Pain points with const enum #37179 (comment) (JS consumers, compile-time vs. runtime version compatibility).

Not inlining enums from referenced projects could be a problem, however. The latest commit solves that by inlining plain enums. Inlining all enums would open up compile-time vs. runtime version compatibility with published .d.ts outputs, but inlining plain enums from referenced projects is safe because they're part of the same build.

The logic is that we inline const enums from .ts files, we use deconstified .d.ts outputs for referenced projects, so inline plain enums from referenced projects.

@jablko jablko changed the title Declare plain enums when preserveConstEnums is on Declare non-const enums if preserveConstEnums is true Mar 2, 2022
@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 16, 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
Archived in project
Development

Successfully merging this pull request may close these issues.

isolatedModules doesn't respect enabled preserveConstEnum option what the project might be build with
3 participants