-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 Request] Add Support for const enum in TS #8741
Comments
Hey @calebeby! We really appreciate you taking the time to report an issue. The collaborators If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack |
@calebeby This is true for cross module enums. There is not way to transpile imported |
@TrySound If |
This is similar to #6476 I believe? Also happy to take a PR for this. If we don't want to simply drop the |
@hzoo export const E = {
A: 1
}; instead of export let E;
(function (E) {
E[E["A"] = 1] = "A";
})(E || (E = {})); because then the output is much less bloated (but it doesn't have the merging ability). I would be happy to look into submitting a PR |
@calebeby I like to place |
@Lodin it would output a plain object, which if you are using scope hoisting in your bundler, terser/uglify could inline the values |
Has any progress been made on this? |
@0xdeafcafe I have not gotten time to work on this yet |
The whole point of import {Direction} from './somewhere';
if (a === Direction.Down) { ... } You need another file access to While compiling
and then read it during compilation of dependent modules. There are still cyclic dependencies to be considered in such design (as when A imports a value from B, while B imports type from A), but it should be already pretty obvious why proposed solution is wrong, and that it's actually quite tedious and ugly task. |
@polkovnikov-ph I will try to explain this again: Input: // a.ts
import {Direction} from './b'
if (a === Direction.Down) {} // b.ts
export const enum Direction {
Up,
Down,
Left,
Right
} Proposed babel output: // a.ts
import {Direction} from './b'
if (a === Direction.Down) {} // b.ts
export const Direction = {
Up: 0,
Down: 1,
Left: 2,
Right: 3
} When compiling Then, after a bundler like Rollup combines them to the same scope: const Direction = {
Up: 0,
Down: 1,
Left: 2,
Right: 3
}
if (a === Direction.Down) {} Then, once a minifier like Terser minifies it: if (a === 1) {} This way, the final output is the same as the The reason this doesn't work for normal enums is because normal enums are created with the ability to merge with other enums with the same name, which makes the code too complex to be hoisted/inlined. var Direction;
(function (Direction) {
Direction[Direction["Up"] = 0] = "Up";
Direction[Direction["Down"] = 1] = "Down";
Direction[Direction["Left"] = 2] = "Left";
Direction[Direction["Right"] = 3] = "Right";
})(Direction = exports.Direction || (exports.Direction = {})); |
@calebeby I'll try to explain again. The whole point of |
If a const enum is used in the same file the transform can be smart enough to inline the enum values. We would only need to create the object for exported enums: it is definetly better than the current "support". |
@polkovnikov-ph I agree with you that if babel could output If I am a user that has a large TS codebase, with many In #6476, another option is proposed, In both cases, the large non-minifyable enum output: var Direction;
(function (Direction) {
Direction[Direction["Up"] = 0] = "Up";
Direction[Direction["Down"] = 1] = "Down";
Direction[Direction["Left"] = 2] = "Left";
Direction[Direction["Right"] = 3] = "Right";
})(Direction = exports.Direction || (exports.Direction = {})); What I am proposing is better than either of those options, because it outputs a minifyable object instead of a non-minifyable function. Although it isn't quite as good as Like @nicolo-ribaudo said, we can implement the inlining behavior in babel if the If enough people think that transpiling cross-file |
How does TypeScript with the |
@nicolo-ribaudo
|
Only for exported const enums? |
It works fine with |
@babel/typescript is it the expected behavior? |
@calebeby I dig this proposal. |
tsc will emit the full "fat enum" format, as The importing file will just have its imports elided, just like type-only imports -- tsc will just leave dangling references to the "concrete" enum. This seems to be the case regardless of the Inputs: /// a.ts
export const enum A {
B
}
export const enum A {
C = 2
}
/// b.ts
import { A } from './a'
console.log(A.B, A.C) Outputs ( /// a.js
export var A;
(function (A) {
A[A["B"] = 0] = "B";
})(A || (A = {}));
(function (A) {
A[A["C"] = 2] = "C";
})(A || (A = {}));
//# sourceMappingURL=a.js.map
/// b.js
console.log(A.B, A.C);
//# sourceMappingURL=b.js.map The tl;dr is that none of this makes any sense. Either typescript needs to find some way for The output for |
- rm - android/app/src/main/res/mipmap-{m,h,x,xx}dpi/ic_launcher.png - ios/*tvOS* - checkout from rn-0.59 - android/gradle/wrapper/gradle-wrapper.properties - android/gradle/wrapper/gradle-wrapper.jar - android/app/BUCK - android/gradlew - android/gradlew.bat - android/app/proguard-rules.pro - merge - android/app/build.gradle - versionCode, versionName - vectorDrawables.useSupportLibrary = true - signingConfigs - android/build.gradle - plugins { id 'base' } - android/settings.gradle - rootProject.name was left as JolocomWallet (instead of older 'jolocomwallet') - android/app/src/main/AndroidManifest.xml - android:allowBackup="true" - android:launchMode="singleTop" - <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/> <uses-permission android:name="android.permission.VIBRATE"/> - 'roundIcon' was unchanged, but we don't have round icons yet - update MainActivity - add 'android:exported="true"' - add intent-filter for consent, payment, authenticate screens - add SpashActivity - android/app/src/main/java/com/jolocomwallet/MainActivity.java - add SplashScreen code - android/app/src/main/java/com/jolocomwallet/MainApplication.java - where's SQLitePluginPackage() ?????? - android/app/src/main/res/values/strings.xml - app_name changes to "Jolocom SmartWallet" - android/app/src/main/res/values/styles.xml - add SplashTheme - app.json - displayName and orientation - tsconfig.json - experimentalDecorators, emitDecoratorMetadata, sourceMap, noUnusedLocals, baseUrl, paths - .babelrc - removed, and moved some config to babel.config.js - add "proposal-decorators" and proposal-class-properties plugins - index.ts - reflect-metadata - metro.config.js - extraNodeModules settings for node polyfills - package.json - adjust startup scripts to use metro - remove all babel dev deps, add new babel deps - remove haul deps - remove 'rnpm' section - add/update deps from new project - react: 16.8.3, - react-native: 0.59.8 - @babel/core: ^7.4.5 - @babel/runtime: ^7.4.5 - @types/jest: ^24.0.13 - @types/react: ^16.8.19 - @types/react-native: ^0.57.60 - @types/react-test-renderer: ^16.8.1 - babel-jest: ^24.8.0 - jest: ^24.8.0 - metro-react-native-babel-preset: ^0.54.1 - react-test-renderer: 16.8.3 - typescript: ^3.5.1 - upgrade some of our old deps: - yarn - ts-jest - react-native-svg - react-native-sqlite-storage - react-native-splash-screen - react-native-camera - react-native-vector-icons - react-native-snap-carousel - remove some unneeded deps: - react-native-svg-uri - add some new deps - @babel/plugin-proposal-decorators - babel-plugin-transform-typescript-metadata - crypto-browserify (polyfill) - assert (polyfill) - stream-browserify (polyfill) - events (polyfill) - vm-browserify (polyfill) - Issues - babel type error - weird "TypeError: t(...).isPlaceholder() is not a function" - worked after removing and reinstalling node_modules - dynamic require in src/locales/i18n.ts - can't use webpack style "dynamic" requires with variables - changed to manual require map - node libs polyfills (crypto, stream, assert, events) - using method from https://github.com/philikon/ReactNativify but just through babel-plugin-rewrite-require and aliases crypto to crypto-browserify - node_modules/web3-eth-accounts/node_modules/eth-lib/lib/bytes.js - if (typeof require !== "undefined") rnd = require("c" + "rypto") <-- WTF - it's probably trying to make sure require is being used from node or something, but in any case "babel-plugin-rewrite-require" does not like this - found some builtin support in metro for mapping modules in a way https://facebook.github.io/metro/docs/en/configuration#extranodemodules - add 'process.version = ...' in index.ts - const enums are not supported in @babel/typescript - it's complicated babel/babel#8741 - removed 'const' from enums in: - src/lib/errors.ts - circular import in storage/entities/index.ts - fixed - decorators were not working correctly due to some babel7+typescript mess - typeorm decorators were breaking on "@context", because of a bug in @babel/plugin-proposal-decorators
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
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
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
One good case for this would be action type names as const enums within a react project which also uses redux - ability to have a HUGE action names enum with essentially zero cost to output bundle is a great benefit. |
Hi guys. I need some help to understand the current situation here please.
Packaged, published. I've kept
The above fails on runtime with Can someone please explain to me why? I've spent two days on the above with no real progress, gone over dozens of SO questions, made over 10 iterations of my library using various TSC compiler options and Babel plugins (such as babel-plugin-const-enum), but this Will.Not.Work. for any reason. It keeps getting transpiled into What am I doing wrong here? I just need to have a common library enum to use across various modules in my project, am I going about it the wrong way? Thank you very much in advance. |
@snathanail The underlying issue is that
|
Understood, thank you very much! I had some hope with It's a pity because I would expect that it's a pretty common scenario. For what it's worth, I ended up dropping PS. I just want to take a moment to address the last part: I hear you, and agree completely, and, believe it or not, 20-year-old me would be jumping for the opportunity to add to the source code. Thanks again for your time and help! |
I opened #13324 to try adding |
@snathanail I am the author of
A possible cause of your error could be if the
|
@polkovnikov-ph
|
I am. I tried both actually (not together). Thank you for devoting time to create this, by the way. 🙂
I've removed the .ts file and the "types" attribute in package.json. Same behavior, except now I'm getting the warning that there is no declaration file (as expected).
I lose the ability to use the enum as a type for another variable, which is what I wanted in the first place. Furthermore, the transpiler output for the library (using the
Using the removeConst options I get this output from transpiling the library:
Again, as expected. Transpiling the main app gives the same output/behavior as before. Note, I've tried both
It does! Interestingly, it transpiles into the very same code (other than the different path for the
It is, I think. This is the full webpack output:
Note, we're getting into webpack internals now, so I'm not 100% sure how this translates to exporting the enum, but from a bit of reverse engineering I guess From all of the above I think I agree with you that there is an issue with importing the const object via webpack. I don't think this has strictly to do with enums. |
@snathanail Thanks for the detailed comment. We are straying away from OP's topic and I don't want to trigger notifications to subscribers of this issue any further.
You can edit your post to link the new post and I can take a look if it's related to my plugin. Your transpiled output doesn't look like the full output. I tested it out locally by appending https://stackoverflow.com/q/48921170/7381355 A few more things to try:
|
Feature Request
In the docs, it says that:
I don't believe that this is true. Babel should be able to transpile
const enum
s into objects, which will achieve the same functionality as typescript, through different transpiled output.Given this code:
tsc
would compile this to:I propose that Babel compiles the enum to:
Which functions the same as the
tsc
output.A minifier like
terser
/uglify
is able to transpile that output ☝️ to the same as thetsc
output.The text was updated successfully, but these errors were encountered: