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

[Feature] Inline const enum values in TypeScript #128

Closed
evanw opened this issue May 21, 2020 · 39 comments · Fixed by #1898
Closed

[Feature] Inline const enum values in TypeScript #128

evanw opened this issue May 21, 2020 · 39 comments · Fixed by #1898

Comments

@evanw
Copy link
Owner

evanw commented May 21, 2020

Right now const enum statements in TypeScript are treated like regular enum statements. This is consistent with how the TypeScript compiler itself behaves when the isolatedModules setting is enabled.

But it'd be nice if the values were inlined, since that would be better for run-time performance and code size. It should be possible for esbuild to do this since it bundles everything end-to-end and has the original TypeScript source code.

I'm not going to do this right now but I'm adding this issue now so I don't forget.

@gregveres
Copy link

+1 in a big, big way. I have const enums all over my automatically generated typescript definitions for my back end. Our backend uses enums in most APIs since the back end is in C#. All of these enums are in namespaces that contain interface definitions and const enum definitions. In our current implementation, all of these things are compile time only entities because the const enums get compiled to const numbers.

@zhusjfaker

This comment has been minimized.

@evanw
Copy link
Owner Author

evanw commented Aug 6, 2020

@zhusjfaker The problem you mentioned is not related to const enums, so it's irrelevant for this issue. Please file a new issue if you have a problem instead of adding on to an existing one. Also for what it's worth this looks to me like a configuration issue with esbuild-loader and Webpack, not with esbuild itself.

@m4rvr
Copy link

m4rvr commented Oct 8, 2020

+1 This would be really cool 😊 Any idea when it will be implemented or still not planned? Thanks!

@Gelio
Copy link
Contributor

Gelio commented Nov 15, 2020

I put some time into this and managed to come up with #534 (helps in case of local const enums)

@DanielRosenwasser
Copy link

Would it be better to to always inline enums regardless of const-ness? This might disincentivize people from using const enums in the first place.

@gregveres
Copy link

@DanielRosenwasser why would always inlining enums dis-incentivize people from using const enums?
Coming from a structured, typed programming language background, enums are a fantastic tool. I find it shocking that javascript developers use strings instead of enums as a widely accepted practice. Enums, compiled down to integers are a great way to increase the expressiveness in my program and to get good type safety at compile time. dissuading people from using const enums is, in my opinion, the wrong way to go.

@leeoniya
Copy link

leeoniya commented Feb 9, 2021

also came up recently here: leeoniya/uPlot#444 (comment)

@jods4
Copy link

jods4 commented Feb 9, 2021

@DanielRosenwasser

Would it be better to to always inline enums regardless of const-ness? This might disincentivize people from using const enums in the first place.

When building for production? Sure, it's smaller and more efficient to inline a non-const enum when possible.

The issue here is that inlining enums (const or not) is hard.

There were some efforts and discussion in #534. You can go have a look but the short version is:

  1. Inlining enums in the same file is done;
  2. Inlining top-level exported enums across files is in theory possible but turned out to be too complicated;
  3. Inlining enums that are inside namespaces requires type analysis and that's something esbuild will probably never do.

I personally think there's a lot of value in 2. and I hope it will eventually be done.

@Gelio
Copy link
Contributor

Gelio commented Feb 9, 2021

I agree inlining enums (const or not) could have benefits in certain situations. It requires probably a deeper analysis, though.

For anyone considering that, my initial thoughts are:

  1. Function calls are not safe to inline, since the functions may not be pure. Also, calling a function multiple times instead of just once, in the enum definition, can be less performant.

    For example, inlining the following may be unsafe

    let state = 1;
    function getNextExponential() {
      return 2 ** (state++);
    }
    
    enum Flags {
      One = getNextExponential(),
      Two = getNextExponential(),
      Three = getNextExponential(),
      Four = getNextExponential(),
      Five = getNextExponential()
    }
    
    const flag = Flags.One | Flags.Five; // 2 ** 1 | 2 ** 5
    // when inlined:
    const flag = getNextExponential() | getNextExponential(); // 2 ** 1 | 2 ** 2
  2. Inlining reference types is unsafe, since the references will be different.

  3. Inlining strings may be worth it if they are short, but for long strings (when the length of the string is larger than the length of the import statement, or some other way of accessing the string) it may result in larger bundle output size.

  4. inlining and erasing enums works only when the enum is used exclusively for known property access. If the enum was inlined and erased, and then some other consumer did Object.values(MyEnum), that would break 😄

Thus, this gets complicated quickly, and requires additional checks for the type of the enum value

@gregveres
Copy link

Why would you consider inlining case you just showed? It's not const and like you said it is not safe.
But the benefit would be huge to inline something simple like this:

export const enum Flags {
 One = 1,
 Two = 2,
  Three = 4,
}

I can say that my use case would be that case alone and nothing else and it would have a large benefit. This is the single construct that is stopping me from using esbuild.

@Gelio
Copy link
Contributor

Gelio commented Feb 9, 2021

@gregveres My comment is a general comment on cases to consider when thinking about inlining enums (const or not, as mentioned by previous commenters in this issue). The snippet is only to show the idea that inlining values that are function calls is unsafe

Personally, I have not used const enums ever in my TS. I have not run any benchmarks, although I would imagine that the difference between using known property access on an enum can be optimized well by JS engines in typical JS apps.

You're saying that the benefit would be huge - did you happen to run any benchmarks to support that opinion?

@gregveres
Copy link

@Gelio No I have not. But even from the amount of code generated and sent down the client there are good wins.

Using enums is great for improved code readability. Using const enums allows you to have long, meaningful identifiers in your source code, knowing that they will be replaced with a single character whenever they are used in your code. And that there is no code generation for the actual const enum itself.

This is not the case with a non-const enum. There is a bunch of code generated for the definition of the enum itself and then there is the identifier left in your code every where you use them.

I have a fairly large app with hundreds of const enums and thousands of references to those enum values.

@jods4
Copy link

jods4 commented Feb 9, 2021

@Gelio I wanted to say the same thing as gregveres.
TS generates ugly enums objects with the forward keys and reverse keys.
Look at this very innocent, 3 values enum and the resulting code:

enum FileMode { Read, Write, ReadWrite }

// Becomes at declaration:
var FileMode;
(function (FileMode) {
    FileMode[FileMode["Read"] = 0] = "Read";
    FileMode[FileMode["Write"] = 1] = "Write";
    FileMode[FileMode["ReadWrite"] = 2] = "ReadWrite";
})(FileMode || (FileMode = {}));

// Becomes at use site:
import { FileMode } from "./other-file";
var x = FileMode.ReadWrite;

// On the other hand, const enum:
// Becomes _nothing_ at declaration
// Becomes at use:
var x = 3; // Might be further inlined or constant-folded by minifier

In my app I have hundreds of const enums (they're generated automatically), so I'd love if the production codegen could use the latter pattern.

Not because I think the code will be faster but because the codegen will be much, much smaller.

@gregveres
Copy link

@jods4 are your enums generated automatically from the backend?
Mine are. My backend is in C# where it is perfectly natural to use enums all over the place and those enums become part of the API and generated automatically. I too would like to get rid of the generated code while taking advantage of the speed of esbuild.

@jods4
Copy link

jods4 commented Feb 9, 2021

@gregveres yes, exactly. We generate a lot of TS front-end stuff from back-end C# sources.

@mindplay-dk
Copy link

Just noticed the crappy output from the official TypeScript compiler for string enums. 🤨

enum Direction {
  Up = "UP",
  Down = "DOWN",
  Left = "LEFT",
  Right = "RIGHT",
}

Compiles to:

var Direction;
(function(Direction2) {
  Direction2["Up"] = "UP";
  Direction2["Down"] = "DOWN";
  Direction2["Left"] = "LEFT";
  Direction2["Right"] = "RIGHT";
})(Direction || (Direction = {}));

When this would do:

var Direction = {"Up":"UP","Down":"DOWN","Left":"LEFT","Right":"RIGHT"};

Man, enums in TS are wonk. I'm inclined to just typing out the whole thing and manually declaring the types instead. 🙄

@gregveres
Copy link

@mindplay-dk Can I ask you a question? I have hundreds of enums in my code. The majority of them are defined in the back end (c#) and then automatically converted to TS and defined as const numeric enums. I have never understood the real attraction of string based enums. I guess I understand that in JS that doesn't have enum support, strings were a reasonable way to create readable code because the string was essentially the name of the enum value. But with TS, where the enum value is an identifier that can be compiled away to a single number, what do you see as the value of using a TS enum with string values?

I am trying to understand so I can be more enlightened and possibly code better. Thx.

@mindplay-dk
Copy link

I have never understood the real attraction of string based enums.

@gregveres Debugging. 🙂

If your program breaks, and your entire program state is just integers - yikes. I wouldn't prioritize micro-performance or memory usage of enums, not by default; not unless we're talking about highly optimized code, like a game or something.

It's also possibly useful for run-time type-checking - like parameter assertions in your functions. If every enum starts at 0, someone could pass a 0 from the wrong enum, but still sneak past a parameter assertion, leading to unpredictable behavior. Technically, strings could do that too, it's just much less likely. Symbol would be ideal for enum values, as they're unambiguous, lightweight and debug-friendly, but may require polyfills with other drawbacks. (Ideally, TypeScript renders all of these points moot - but in practice, you are going to have run-time errors, since static typing doesn't help with data you ingest from external sources.)

@gregveres
Copy link

@mindplay-dk That makes sense. Thanks.

@AlonMiz
Copy link

AlonMiz commented May 19, 2021

+1
this is our topmost blocker in order to move to esbuild

@rsms
Copy link

rsms commented May 25, 2021

Working with a client with a large TS codebase, converting their babel-based system to esbuild and we ran into an issue that turned out to be caused by this behavior. Essentially they have a number of modules, e.g. "common-stuff" and "app1" where "app1" imports things from "common-stuff" which in turn uses export const enum Things { ... }.

They are using some features that prohibit isolatedModules (TS1208, TS2748), e.g. TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided.

Getting inlining of const enum values into esbuild would be *~lovely~*

@evanw
Copy link
Owner Author

evanw commented May 25, 2021

In general esbuild requires the isolatedModules flag because that avoids type-only TypeScript constructs. Since esbuild doesn't have a type system, it doesn't make use of information that is only contained in type annotations. Everything inside of declare (i.e. an "ambient context") is a type annotation.

I believe that TS2748 is caused by the use of export declare const enum instead of export const enum because "ambient" means declare. In that case it should be possible to get their code to compile by removing the declare. I just gave that a try and it seems to validate the cause and the fix:

// enum.ts
export declare const enum Foo { A, B }
// entry.ts
import { Foo } from "./enum"
console.log(Foo.A)

If I compile this using tsc with isolatedModules: true I get error TS2748: Cannot access ambient const enums when the '--isolatedModules' flag is provided but it works fine if I compile this after removing declare. This is the same with esbuild (it works without the declare but not with the declare).

I'm wondering how this worked with that client since Babel doesn't seem to support const enum at all? I just get 'const' enums are not supported when I try it. Maybe they are using the official TypeScript compiler. If so, that could still be an option (run tsc first then bundle with esbuild). That would of course lose most of the speed benefit of esbuild but if you still want to rely on type-only TypeScript features, you may need the official TypeScript compiler.

@leeoniya
Copy link

I'm wondering how this worked with that client since Babel doesn't seem to support const enum at all?

perhaps via https://github.com/dosentmatter/babel-plugin-const-enum ?

@rsms
Copy link

rsms commented May 25, 2021

There was/is a mix of ts-loader, tsc -b and babel (with plugins) in use within the codebase I'm referring to.
Anyhow, we decided that changing exported const enums to simply enums was the right move. The practical code size impact and performance impact should be minimal.
Evan, thank you for all your amazing work!

@calebeby
Copy link
Contributor

calebeby commented Sep 2, 2021

FWIW Babel has now implemented this directly: babel/babel#13324

@Gelio
Copy link
Contributor

Gelio commented Sep 3, 2021

FWIW Babel has now implemented this directly: babel/babel#13324

From that PR:

  • For const enums only used in the same file where they are declared, we can safely inline the enum values. This matched the TS behavior without --isolatedModules, and produces a much smaller output.

AFAIR this is essentially what I did in #534. Sadly, there is no attention paid to that PR.

@jods4
Copy link

jods4 commented Sep 3, 2021

And they went a little bit further:

  • When the const enum is exported, we can generate an object mapping from the enum keys to its values. This is way smaller than the default enum output, because it doesn't need to support reverse mappings (i.e. E[E.x]).

It's not perfect but it's a welcome improvement over the crazy default enum codegen from TS.

@Gelio
Copy link
Contributor

Gelio commented Sep 3, 2021

It's not perfect but it's a welcome improvement over the crazy default enum codegen from TS.

Yup, good point. I suppose I could include that in #534 if there's interest in that.

@lxsmnsyc
Copy link

I was about to file an issue and I found this. So it seems that const enum is partially working: the values are inlined when accessing the enum, however, the output is still the same to that of a regular enum if the enum is exported (i.e. export const enum).

@nisimjoseph
Copy link

Is any update on const enum not building correctly?
this is a blocker to move to ESBuild.

@evanw
Copy link
Owner Author

evanw commented Dec 21, 2021

I just shipped cross-module enum inlining. Details are in the release notes: https://github.com/evanw/esbuild/releases/tag/v0.14.7. I have not yet updated tree shaking to account for this though, so using enums this way will currently still result in potentially-unused generated enum definition objects. I will consider this feature to be complete once tree shaking is updated.

@eamodio
Copy link

eamodio commented Dec 22, 2021

@evanw I just upgrade the GitLens codebase to 0.14.7 to take advantage of the new enum inlining, but when I look at the built output I still see enum references rather than inlined values.

I've seen it with numeric enums like:

export const enum ReferencesQuickPickIncludes {
	Branches = 1 << 0,
	Tags = 1 << 1,
	WorkingTree = 1 << 2,
	HEAD = 1 << 3,

	BranchesAndTags = Branches | Tags,
}

And string enums:

export const enum ViewBranchesLayout {
	List = 'list',
	Tree = 'tree',
}

Am I missing something, or is my setup getting in the way (using webpack and the esbuild-loader)? Or something else?

@evanw
Copy link
Owner Author

evanw commented Dec 22, 2021

This benefit is only for people using esbuild as a bundler. It won't work if you're using another bundler since the inlining happens during bundling.

@eamodio
Copy link

eamodio commented Dec 23, 2021

Damn, OK thanks 😄

@evanw evanw closed this as completed in c8781b5 Dec 28, 2021
@evanw
Copy link
Owner Author

evanw commented Dec 29, 2021

Tree shaking for cross-module inlined enum values has been implemented. This feature should now work the way I intend for it to work. Specifically:

  1. There is no difference between enum and const enum in esbuild
  2. Accessing an enum value directly means it will be inlined (as long as the value is a number or a string)
  3. Referencing the enum itself will cause the code for the enum object map to be included in the output
  4. Cross-module inlining only works when bundling
  5. You have to use ESM import/export syntax for cross-module inlining to work

@71
Copy link

71 commented Jun 17, 2023

I've found an edge case for this (not filing a separate issue as I'm not sure you want to support it): const enums in declare'd namespaces are ignored, rather than inlined. Code:

namespace A {
    export enum Foo { A, B }
    export const enum Bar { C, D }
}

declare namespace B {
    export enum Foo { A, B }
    export const enum Bar { C, D }
}

console.log(A.Foo.A, A.Bar.C, /* not defined: `B`: B.Foo.A, */ B.Bar.C);

The code above compiled with TS successfully runs (as B.Bar.C is replaced by 0), but with esbuild it encounters an error "ReferenceError: B is not defined" (as B.Bar.C is left as-is).

@evanw
Copy link
Owner Author

evanw commented Jun 20, 2023

I believe the problem there is declare namespace. That makes B a type annotation, which requires a type system to process, and esbuild does not have a type system. So that's why esbuild ignores B. This limitation is by design, and is documented here: https://esbuild.github.io/content-types/#no-type-system.

You can see this for yourself on the TypeScript playground. If you turn on isolatedModules: true (which you are supposed to be doing), the TypeScript compiler will reject your code with the following error:

Cannot access ambient const enums when 'isolatedModules' is enabled.

You can use esbuild and TypeScript without isolatedModules: true and some things will happen to work, but things aren't guaranteed to work. If you want the TypeScript compiler to tell you about problems like this at compile time instead of finding out about them at run-time, you should enable isolatedModules: true.

@71
Copy link

71 commented Jun 23, 2023

Got it, thanks! Thank you also for the pointer to isolatedModules; since the example I wrote doesn't use any module I wouldn't have found that on my own. It does make sense that it would "ignore" file-local type declarations to be consistent.

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 a pull request may close this issue.