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

isolatedModules doesn't respect enabled preserveConstEnum option what the project might be build with #37774

Open
5 tasks done
timocov opened this issue Apr 3, 2020 · 13 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@timocov
Copy link
Contributor

timocov commented Apr 3, 2020

Search Terms

isolatedModules, preserveConstEnum, const enum

Suggestion

The compiler should respect/understand that the package/module was compiled with enabled preserveConstEnums compiler option.

Use Cases

Let's say we have a project written in TypeScript, which uses const enums.

The project uses them because the main const enum's advantage is inlining values instead of runtime accessing to the value.

In other hand the maintainers understand that const enums compiles in nothing in JS code, and that means that consumers can't use a const enums in their JS code (in some cases in TS code either, see above). Thus, they decided to enable preserveConstEnums compiler option to leave all const enums in JS code and allow consumers use them as well.

Up to this moment everything is good. But the next who appears on the stage is isolatedModules (say hello to create-react-app, webpack's transpile-only mode, transpileModule compiler's API and so on). This compiler option is used to detect that imported module could be transpiler without compilation into JS code and everything will work well. One of checks for this option is that you don't import const enums (error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided).

To summarize, if the project/package is compiled with enabled preserveConstEnums, in isolatedModules mode the compiler shouldn't fail with error about unavailability of using const enums, because that's not actually true.

Examples

Quite possible we need to provide that information to the compiler somehow (by providing tsconfig.export.json or in package.json for instance), but I don't even know how it would be done.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@RyanCavanaugh
Copy link
Member

It seems like arguably preserveConstEnum should just emit enum instead of const enum to the .d.ts, though I'm sure that would break some other scenario

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Apr 3, 2020
@timocov
Copy link
Contributor Author

timocov commented Apr 3, 2020

though I'm sure that would break some other scenario

@RyanCavanaugh Yeah, at least a consumer who uses TypeScript without using isolatedModules couldn't get that "enums" inlined 🙂

@ok-AK
Copy link

ok-AK commented Nov 2, 2020

Any updates for this issue?

CRA hard enforces the isolatedModules: true flag nowadays. It used to be possible to extend the tsconfig to override this but now it looks like eject or replace the react-scripts file specific to enforcing tsconfig. Neither is desirable when the issue is strictly the way preserveConstEnum is behaving. Running into this issue right now so curious. Thanks!

@splincode
Copy link

i have same issue

@MLoughry
Copy link

I'm hitting this issue as well trying to enable the isolatedModules flag.

@MLoughry
Copy link

@RyanCavanaugh I see this is tagged with Awaiting more feedback. What feedback are you looking for?

@andrewbranch
Copy link
Member

We’ve discussed this some in the past and I feel like I must be forgetting some of the complexities that came up. I just jotted down some straw-man proposals, and the simplest one feels fairly straightforward:

preserved modifier for const enums

// @Filename: /node_modules/foo/index.d.ts
export preserved const enum Foo {
  Bar = 1
}

// @Filename: /index.ts
// @isolatedModules: true
import { Foo } from "foo";
Foo.Bar; // ok
  • Only valid in ambient contexts, automatically added to declaration emit under --preserveConstEnums.
  • A preserved const enum can be imported and used under --isolatedModules because its usage need not be inlined.
  • If needed, a compiler flag could be added that controls whether preserved const enums from node_modules / other projects / declaration files get inlined or not. (We’ve had some concerns before about inlining enum values from declaration files that could get out of sync with implementations, but I want to hear the case for that again before I’d be enthusiastic about adding a flag.)

I tossed around another proposal combining @RyanCavanaugh’s original suggestion that const enums could be emitted to declaration files as regular enums with a compiler option --inlineEnums that could offer local control over what enums get inlined, but ultimately I think that level of control would be more confusing than useful. Part of our hesitation on doing anything about this is that enums are already confusing, so we want a solution to be as simple and explainable as possible.

@jablko
Copy link
Contributor

jablko commented Nov 4, 2021

There are three issues with exported ambient const enums, I think:

  1. They're incompatible with isolatedModules.

    tsc --declaration stops downstream consumers from using isolatedModules, roughly. Specifically, by transforming non-ambient const enums (.ts files) into ambient const enums (.d.ts files), it precipitates the following trilemma:

    • upstream project can use const enums
    • downstream consumer can use those enums' values
    • downstream consumer can use isolatedModules

    Choose any two. 😞

  2. 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:

  3. Import elision. importsNotUsedAsValues: "preserve" preserves imports for inlined enum values, but ambient const enums (.d.ts files) may or may not have runtime .js files. The usual way to unambiguously elide imports, import type, doesn't allow const enum values, currently.

    Taking the opposite approach (eliding imports for inlined enum values) would invalidate importsNotUsedAsValues: "preserve" for ambient const enums with runtime .js files.

These are the solutions as I see them:

  • A. Don't use const enums entirely. Ban them with a linter.

    Avoids #1 and #2, sidesteps #3, but stops upstream projects from inlining enum values, with performance implications.

    Doable today.

  • B. Transform non-ambient const enums into ambient non-const enums, either with:

    Same impact as A except upstream projects can inline enum values. Works with and without isolatedModules. Consumers can't inline enum values (:+1: good, per #2).

  • C. Want consumers to inline enum values, with consequent isolatedModules incompatibility, maybe because all participating projects are part of an internal ecosystem/composite project where versions are synchronized? Then preserveConstEnums: false is for you!

    Know and accept the implications: #1, #2 and #3.

    No change: Same as today.

  • X. Want some consumers to inline enum values and others not to, or another advanced scenario? Out of scope, for now:

    local control over what enums get inlined, but ultimately I think that level of control would be more confusing than useful. Part of our hesitation on doing anything about this is that enums are already confusing, so we want a solution to be as simple and explainable as possible.

Note: Existing preserveConstEnums and isolatedModules behavior

Because I TIL'ed :-)

Turns off inlining Preserves imports Transforms enums -> .js
preserveConstEnums ✔️
isolatedModules ✔️ ✔️ ✔️

I.e. isolatedModules implies preserveConstEnums, !preserveConstEnums implies !isolatedModules.

@dmichon-msft
Copy link
Contributor

I've been giving significant thought to injecting a custom transformer into the pipeline that converts this:

// Current const enum
export const enum AstKind {
    None = 'None',
    Script = 'Script',
    AndIf = 'AndIf',
    Command = 'Command',
    CompoundWord = 'CompoundWord',
    VariableExpansion = 'VariableExpansion',
    Text = 'Text'
}

Into this:

// Provide values at runtime, use an object literal to allow minifiers to drop unused values and/or inline (if in the same scope, at least)
export const AstKind = {
    None: 'None',
    Script: 'Script',
    AndIf: 'AndIf',
    Command: 'Command',
    CompoundWord: 'CompoundWord',
    VariableExpansion: 'VariableExpansion',
    Text: 'Text'
} as const;
// Allow the values to be used in a type position
export declare namespace AstKind {
    export type None = typeof AstKind.None;
    export type Script = typeof AstKind.Script;
    export type AndIf = typeof AstKind.AndIf;
    export type Command = typeof AstKind.Command;
    export type CompoundWord = typeof AstKind.CompoundWord;
    export type VariableExpansion = typeof AstKind.VariableExpansion;
    export type Text = typeof AstKind.Text;
}
// Allow for the enum itself ot be used in a type position
export type AstKind = typeof AstKind[keyof typeof AstKind];

This allows the following usage:

import { AstKind } from './AstKind';
const x: AstKind.AndIf = AstKind.AndIf;
const y: AstKind = AstKind.Command;

Also further enables the following, which you can't do with a const enum:

import { type AstKind } from './AstKind';
// Type checks properly
const z: AstKind.Text = 'Text';
const z: AstKind = 'Text';

but still prevents, for example:

const x: AstKind.AndIf = AstKind.Command; // Error, not assignable
const y: AstKind = 'Unknown'; // Error, not assignable

The one restriction of current const enum that gets lost is that it allows cross-assignment of enums as long as the values are compatible, but this is as much a feature as it is a bug, since it is what allows type-only imports to the enum.

It is possible to restore the assignability restriction while still allowing the raw string literal assignment via something like:

export declare const __enum__OtherEnum: unique symbol;
export const OtherEnum = {
    Bar: 'None',
    Baz: 'Script',
    Foo: 'Test'
} as {
    Bar: 'None' & { __enum?: typeof __enum__OtherEnum };
    Baz: 'Script' & { __enum?: typeof __enum__OtherEnum };
    Foo: 'Test' & { __enum?: typeof __enum__OtherEnum };
};
export declare namespace OtherEnum {
    export type Bar = typeof OtherEnum.Bar;
    export type Baz = typeof OtherEnum.Baz;
    export type Foo = typeof OtherEnum.Foo;
}
export type OtherEnum = typeof OtherEnum[keyof typeof OtherEnum];

Though unfortunately that probably has deleterious effects on type-checking performance. It is also a lot less readable than an enum OtherEnum { ... } declaration.

@octogonz
Copy link

octogonz commented Apr 6, 2022

preserved modifier for const enums

Maybe the keyword should be inline. It seems that the underlying concept is:

  • The person who created this enum thinks it's a good idea for compilers to inline its members
  • However there's no obligation to do so; the runtime object will always exist

That is exactly the meaning of inline in C and C++.

In fact const enum is somewhat counterintuitive, given that enums are always "constants" (in common understanding).

It would also simplify adoption. The message is: "search+replace const enum with inline enum everywhere and you're good."

And then later we could introduce inline functions. 😉

@bradzacher
Copy link
Contributor

We run into this error at work because we define all our enums as const and our entire codebase is put together using project references.

The crux of the issue, from what I can tell, is that TS treats project references as an "opaque boundary" and ignores the tsconfig settings that may be set.

This is one of those "principle of least surprise" things I suppose - because there's no guarantee that a referenced project is built using the referenced tsconfig settings so in order to be least surprising (and cause fewer runtime crashes) it's better to assume the const enum is always elided.

However this does mean that in codebases that are built using the same config/built tool setup there's no way to convince TS that the pattern is in fact completely safe. So you're left with a typecheck run that's littered with TS2748 errors.

At Canva we work around this by setting noEmitOnError: true (along side isolatedModules: true, preserveConstEnums: true) and we don't run tsc directly and instead have a wrapper which deletes the TS2748 error from the output.
It's an ugly workaround because it means we have to build all our code using the wrapper. Thankfully VSCode doesn't show this specific error in the IDE (#57833) or else we'd be in a difficult position and have to either convert all enums to non-const enums or disable isolatedModules in our root config.

I wonder if there's a simple solution here to allow a user to convince TS that accessing const enum across the boundary is in fact safe? allowConstEnumAcrossBoundaryTrustMeBro: true?

@andrewbranch
Copy link
Member

Project references are not always considered as an opaque boundary. Module resolution in referenced .d.ts files, for example, happens according to the referenced project’s settings, not the importing project’s settings, and we’ve speculated at times that there may be other places in the checker where we should be fetching the referenced project’s settings but are currently using the currently checked project’s settings. It doesn’t seem out of the question to me that this error could be silenced when it can be determined to be safe due to the ambient file belonging to a project that uses preserveConstEnums.

@jakebailey
Copy link
Member

I wanted something like this in #51530 so that we could use const enums within TS but still refer to them by value if needed; my issue is probably a dupe of the above request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.