Skip to content

Pain points with const enum #37179

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

Closed
michaelsbradleyjr opened this issue Mar 3, 2020 · 4 comments · Fixed by microsoft/TypeScript-Website#2127
Closed

Pain points with const enum #37179

michaelsbradleyjr opened this issue Mar 3, 2020 · 4 comments · Fixed by microsoft/TypeScript-Website#2127
Labels
Discussion Issues which may not have code impact

Comments

@michaelsbradleyjr
Copy link

At the express suggestion of a package maintainer whose package has over 36 million weekly downloads, I submit the following (original submission/comment is chalk/chalk#373 (comment)):


I wrote:

@babel/preset-typescript gets about 3.7 million weekly downloads; typescript gets about 7.9 million.

This is not a rigorous analysis... but, because the problem arises with const enum being unsupported by @babel/preset-typescript (because it only works with code that conforms to isolatedModules), that means about half of the typescript projects out there can't use chalk 3.x.

@babel/preset-typescript also doesn't fully support TS namespaces. The TS maintainers have publicly said that ES6 modules are the future; on the other hand, they've also publicly said they have zero plans to remove namespaces from the language. I don't know for certain, but I expect they would not be open to removing const enum either.

There is a plugin (babel-plugin-const-enum) that can help with the const enum situation. However, it's not a wonderful solution because those of us who use tsc to typecheck — which is everyone since babel/preset-typescript doesn't do any typechecking — need to disable the isolatedModules option. But it's important to have that option in place in projects using @babel/preset-typescript, especially in larger projects, to ensure that contributors don't commit code that won't work with it.

In summary: many TypeScript projects won't be able to use chalk 3.x if it retains const enum. It's not really any one's fault, it's just how things are given the technical constraints of @babel/preset-typescript and the practical need to typecheck with tsc in a manner that detects code that isn't compatible.

I'm sure none of the above will convince you to change your mind, and there's certainly no resentment on my part. I wasn't sure how many people who encounter this issue understand the core of the problem, so I hope my comment is helpful. Thanks for all your hard work on this wonderful library!


The maintainer replied:

They don't need to remove it, just officially discourage it ... const enum is a real pain-point for users. You can fix Chalk here, but there will be lots of other projects using const enum and are you going to spend time on this every time you encounter a project using const enum?


Is there something I'm missing as a consumer of this library running into problems with isolatedModules clashing with const enum?

Is there something the package maintainer could do to alleviate the problem in a manner that doesn't sacrifice the performance gains of const enum?

Is there something the TypeScript maintainers can do to help improve the situation?

cf. #30590 and facebook/create-react-app#4837 (comment).

Much thanks for your time and consideration!

@RyanCavanaugh RyanCavanaugh added the Discussion Issues which may not have code impact label Mar 3, 2020
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 3, 2020

I think the linked PR is bad very good. const enum implies there's no corresponding runtime object, but we do have a runtime object corresponding to this declaration.

Moreover, the package docs imply that it's expected to write { level: 2 } in code - it seems like a symbolic name for these values is not strictly necessary.

So it seems like the status quo proposed PR is the best of all worlds -- users (both TS and JS!) can refer to the levels through symbolic names if they want, or use the documented 0-3 values if they really need to save a dozenish bytes of package size.

The intended use case of const enum would be for a value that probably doesn't even escape the file it's declared in, or at the very least doesn't escape the project it's declared in. Leaking a const enum outside your project scope is very dangerous for versioning because you can easily get into a state where two people who wrote code referring to the same symbolic name end up with different values.

Really all we can do is ask that people understand the implications of const enum - just like you wouldn't ask JS consumers to run a custom Babel transformer to use your library, you shouldn't expect TS consumers to consume const enums unless there's some very good reason. Provide symbolic, non-inlined names through regular enums if that's desired, or use nonsymbolic values like 2 if those make sense.

@RyanCavanaugh
Copy link
Member

Note: I originally misread which direction the linked PR changes things, so please reread the above comment if you read it earlier.

@sindresorhus
Copy link

const enum implies there's no corresponding runtime object, but we do have a runtime object corresponding to this declaration.

This was not intended: chalk/chalk#373 (comment)

The intended use case of const enum would be for a value that probably doesn't even escape the file it's declared in, or at the very least doesn't escape the project it's declared in.

This is not clear in the docs either.

Really all we can do is ask that people understand the implications of const enum

It's hard to understand implications that is not clear by its docs.

you shouldn't expect TS consumers to consume const enums unless there's some very good reason.

What are you saying here? That const enum should not be used in ambient files at all?

@sindresorhus
Copy link

sindresorhus commented Mar 8, 2020

const enums are often misused. - facebook/create-react-app#4837 (comment)

Maybe this is a hint that the docs are not good enough? It's not like us users want to use something incorrectly.

lucaswerkmeister added a commit to wmde/WikibaseDataModelTypes that referenced this issue Jun 5, 2020
We don’t want to use const enums, for two reasons:

1. It seems to be generally discouraged to share const enums across
   module boundaries; they are apparently mostly intended as an internal
   mechanism. (I have yet to find any documentation that tells you this,
   but [1] seems to imply we’re supposed to understand this naturally.)

2. Babel’s TypeScript transform plugin does not support them [2]. There
   is a plugin [3] to transform const enums into non-const ones, but it
   seems to be intended for your own code, not for dependencies. This
   means that Data Bridge can’t use this module as long as it uses const
   enums, because they will not be inlined at compile time, but rather
   be sought for at run time, when they don’t exist.

With type aliases for union types, we lose the possibility for
declaration merging, but it’s unclear how significant this actually was.
If we (the Wikidata team) need more data (value) types, it’ll make more
sense to add them upstream (here) anyways, rather than augmenting the
declarations in our downstream projects.

(Side note: the augmentation syntax in the README.md was incorrect
anyways – a namespace name must be an identifier, not a string literal.
An ambient module would have been closer to the real syntax.)

[1]: microsoft/TypeScript#37179 (comment)
[2]: babel/babel#8741
[3]: https://github.com/dosentmatter/babel-plugin-const-enum
lucaswerkmeister added a commit to wmde/WikibaseDataModelTypes that referenced this issue Jun 5, 2020
We don’t want to use const enums, for two reasons:

1. It seems to be generally discouraged to share const enums across
   module boundaries; they are apparently mostly intended as an internal
   mechanism. (I have yet to find any documentation that tells you this,
   but [1] seems to imply we’re supposed to understand this naturally.)

2. Babel’s TypeScript transform plugin does not support them [2]. There
   is a plugin [3] to transform const enums into non-const ones, but it
   seems to be intended for your own code, not for dependencies. This
   means that Data Bridge can’t use this module as long as it uses const
   enums, because they will not be inlined at compile time, but rather
   be sought for at run time, when they don’t exist.

With type aliases for union types, we lose the possibility for
declaration merging, but it’s unclear how significant this actually was.
If we (the Wikidata team) need more data (value) types, it’ll make more
sense to add them upstream (here) anyways, rather than augmenting the
declarations in our downstream projects.

(Side note: the augmentation syntax in the README.md was incorrect
anyways – a namespace name must be an identifier, not a string literal.
An ambient module would have been closer to the real syntax.)

[1]: microsoft/TypeScript#37179 (comment)
[2]: babel/babel#8741
[3]: https://github.com/dosentmatter/babel-plugin-const-enum
lucaswerkmeister added a commit to wmde/WikibaseDataModelTypes that referenced this issue Jun 8, 2020
We don’t want to use const enums, for two reasons:

1. It seems to be generally discouraged to share const enums across
   module boundaries; they are apparently mostly intended as an internal
   mechanism. (I have yet to find any documentation that tells you this,
   but [1] seems to imply we’re supposed to understand this naturally.)

2. Babel’s TypeScript transform plugin does not support them [2]. There
   is a plugin [3] to transform const enums into non-const ones, but it
   seems to be intended for your own code, not for dependencies. This
   means that Data Bridge can’t use this module as long as it uses const
   enums, because they will not be inlined at compile time, but rather
   be sought for at run time, when they don’t exist.

With type aliases for union types, we lose the possibility for
declaration merging, but it’s unclear how significant this actually was.
If we (the Wikidata team) need more data (value) types, it’ll make more
sense to add them upstream (here) anyways, rather than augmenting the
declarations in our downstream projects.

(Side note: the augmentation syntax in the README.md was incorrect
anyways – a namespace name must be an identifier, not a string literal.
An ambient module would have been closer to the real syntax.)

[1]: microsoft/TypeScript#37179 (comment)
[2]: babel/babel#8741
[3]: https://github.com/dosentmatter/babel-plugin-const-enum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants