-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Support TypeScript const enums #13324
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2831419:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46202/ |
packages/babel-preset-typescript/test/normalize-options.spec.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
@@ -56,6 +57,7 @@ export default declare((api, opts) => { | |||
jsxPragma = "React.createElement", | |||
jsxPragmaFrag = "React.Fragment", | |||
onlyRemoveTypeImports = false, | |||
optimizeConstEnums = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we default it to true
in next minor? I know we have onlyRemoveTypeImports
default to true
in Babel 8, but supporting new language features should be opt-in by default, like we did for other ES features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should.
In TypeScript with --isolatedModules
, code like this works and logs x
:
const enum A {
x
}
// @ts-expect-error
console.log(A[A.x]);
export {};
However, with optimizeConstEnums
it stops working because we don't ignore const
anymore.
For the default behavior, I'd prefer to match --isolatedModules
as closely as possible where it's easy to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I think it should be a TS issue, they could have optimized the const enums here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript docs mentions why ambient const enum
s (ie. declare const enum
) don't work with isolatedModules
.
When isolatedModules: false
, it seems const enum
and declare const enum
function the same.
When isolatedModules: true
, it seems const enum
transpiles as if it was just enum
since you get the reverse-mapped object. declare const enum
transpiles into the usual empty output, because it's expected to be inlined. However, the isolatedModules: true
will produce error:
Cannot access ambient const enums when the '--isolatedModules' flag is provided.(2748)
No idea why they only document declare const enum
and not const enum
. But I assume their reasoning of converting const enum
to just enum
is to have it work across modules. And I guess they treat declare const enum
as an error since you are explicitly saying it is ambient.
Anyway, @nicolo-ribaudo. I'm not familiar with babel code, but did you make sure you also support declare const enum
when optimizeConstEnums: true
? When isolatedModules: false
, declare const enum
seems to function the same as const enum
, so you can just strip the declare
.
Also, if we want to copy TypeScript functionality with isolatedModules: true
and optimizeConstEnums: false
, should we transpile const enum
by treating it as enum
ie. transpile it to the reverse-mapped object instead of throwing an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default Babel behavior is to match --isolatedModules
(as mentioned in our docs at the 4th caveat).
Thus, this PR makes it so that by default const enum
s are compiled to "normal" enums, which also matches the default behavior of your babel-plugin-const-enum
plugin. It's implemented in 933ba59
(#13324).
With optimizeConstEnums: true
the plugin seems to correctly work: https://github.com/babel/babel/pull/13324/files#diff-d0d0efefe2a94935f80a8f399bbca77e2a0fbbc8e7a3ba2a1a2f07eff7bcb0de. When optimizeConstEnums: false
, it produces some broken output:
declare const enum Animal {
Dog,
Cat
}
let x: Animal = Animal.Dog;
// becomes
let x = Animal.Dog;
however, I'm not too much concerned about it because:
- It's exactly the same output that TS generates with
--isolatedMoudles
- TypeScript will report a type-checking error saying that
declare const enum
are not supported.
When using
--isolatedModules
, TypeScript compilesconst enum
declarations as if they were normal enums: https://www.typescriptlang.org/play?#code/MYewdgzgLgBApmArgWxgQRgbwFAxgD2wF9ts58AHEAJ1lElgRRgCEtcYBPGAXhgCZipNADp8AbmyjO4oA (click on "TS Config" and enableisolatedModules
). We can do the same thing by default, rather than throwing.Additionally, this PR introduces a new
optimizeConstEnums
option to the TS plugin and preset:const enum
s 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.const enum
is exported, we can generate an object mapping from the enum keys to its values. This is way smaller than the defaultenum
output, because it doesn't need to support reverse mappings (i.e.E[E.x]
).