Change enums to `const` to make types package compile away

[xirzec]

Is your feature request related to a problem? Please describe. Right now when I import @opentelemetry/types, I still have to configure rollup (the bundler my project uses) to handle named exports for enums like SpanKind and CanonicalCode. This is a little painful for transitive dependencies in my project.

Describe the solution you'd like If you add the const keyword to your enums, then TS will substitute uses of the enum with the literal value during compilation. This allows TS to not have any runtime artifact of the enum, which to me feels a lot better, especially for a types-only package.

This would eliminate my need to worry about making the enum available at runtime.

[dyladan]

export enum A {
  ONE = "one",
  TWO = "two"
}

export const enum B {
  ONE = "one",
  TWO = "two"
}

console.log(A.ONE);
console.log(B.ONE);

compiles to

"use strict";
exports.__esModule = true;
var A;
(function (A) {
    A["ONE"] = "one";
    A["TWO"] = "two";
})(A = exports.A || (exports.A = {}));
console.log(A.ONE);
console.log("one" /* ONE */);

[obecny]

It seems like this is not so straightforward. Changing enum to be a const removes the ability to use the enum object as lookup object which for example is used here:

1. https://github.com/open-telemetry/opentelemetry-js/blob/a411313cb30823cce8ea19d9de2f2c8ab69cbef2/packages/opentelemetry-exporter-zipkin/src/transform.ts#L78

2. https://github.com/open-telemetry/opentelemetry-js/blob/a411313cb30823cce8ea19d9de2f2c8ab69cbef2/packages/opentelemetry-exporter-zipkin/test/transform.test.ts#L137

[Flarna]

Using const enums could be also an issue for pure JS consumers as they have to extract the actual value from source code then. This could be avoided by using typescript compiler option preserveConstEnums. Typescript consumers get the values inlined but JS consumers can still import the package and reference the generated enum object.

[obecny]

Ok so far it looks like this

1. export enum CanonicalCode , preserveConstEnums: false lookup object works, js file contains data

2. export enum CanonicalCode , preserveConstEnums: true lookup object works, js file contains data

3. export const enum CanonicalCode , preserveConstEnums: false lookup object fails, js file empty

4. export const enum CanonicalCode , preserveConstEnums: true lookup object fails, js file contains data

So if I change any enum to const no matter what the lookup object fails, which means I should not change it. If there is already an enum defined as const then lookup object fails (but obviously is not being used like this), but the js file will contains the data instead of being empty.

Summarising: Correct me if I'm wrong but the only way that I can do is to add preserveConstEnums: true to tsconfig but I will not change all enums definition to be const.

@open-telemetry/javascript-approvers ^^

[Flarna]

Actually lookup works for const enums with preserveConstEnums: true if the module containing the exported enum is present. But typescript compains with message A const enum member can only be accessed using a string literal. so users have to cast.

[obecny]

@Flarna ...so users have to cast can you be more specific ? How will you fix then this line for example: https://github.com/open-telemetry/opentelemetry-js/blob/a411313cb30823cce8ea19d9de2f2c8ab69cbef2/packages/opentelemetry-exporter-zipkin/src/transform.ts#L78

[Flarna]

This should work tags[statusCodeTagName] = String((types as any).CanonicalCode[status.code]);.

But now having it in front of me I noticed that it is that ugly to state that it should be not considered at all...

[dyladan]

I would prefer only setting const on enums which not used as lookup enums. String((types as any).CanonicalCode[status.code]); is just ugly. Correct me if I'm wrong, but preserveConstEnums: true defeats the entire point, because then the package is not compiled away. I think this is a very minor optimization anyways so it's probably not worth adding weird workarounds to make it work. I would add const where it is possible to do it without other changes and leave all other enums as is.

[xirzec]

A design question I have -- if folks find themselves needing to use enum maps to coerce enum values to strings a lot, why is the enum in question not a string enum to begin with? Is it to save bytes over the wire or something?

[markwolff]

As per conversation w/ @xirzec, an issue is that rollup does not understand nonconst enums since they are set inside a runtime function instead of being able to be checked staticly. Therefore they have to manually define each enum this repo ever defines as a namedExport inside each rollup config, regardless of if their lib ever uses those enums. e.g.

cjs({
  namedExports: {
    "@opentelemetry/types": ["CanonicalCode", "SpanKind", "TraceFlags"]
...

I assume webpack could have a similar issue?

[MSNev]

As I put in my version (duplicate #1753) and I see that @dyladan mentions above, we should only do this if reverse lookup is not required (which should be the default in my opinion) and if we really want reverse lookup then we should not (necessarily) define it as an enum anyway and instead define as a const object so it's explicit that we are "allowing" the reverse lookup.

export const A = { ONE = 1, TWO = 2 }

becomes

var A = { ONE: 1, TWO: 2 };

rather than the function wrapper as highlighted above

And also don't use preserveConstEnums: true IMHO

[pauldraper]

const enums are a TypeScript misfeature.

They force downstream consumers to have a TypeScript compiler that reads and inlines the value.

And they offer no real benefit. Doesn't make code faster. Saves a scant handful of bytes.

[dyladan]

There is no more types package so I think this should be closed.

Const enums don't require downstream to have typescript. They compile to point of use string constants. That's the whole point. The absolutely miniscule performance benefit that may exist is not worth the trade off imo

[pauldraper]

Const enums don't require downstream to have typescript. They compile to point of use string constants.

Apparently it doesn't matter anymore, but FYI..."compiling to point of use" requires the point of use to have a compiler. Pure JS cannot consume anything that's a const enum.

[dyladan]

Const enums don't require downstream to have typescript. They compile to point of use string constants.

Apparently it doesn't matter anymore, but FYI..."compiling to point of use" requires the point of use to have a compiler. Pure JS cannot consume anything that's a const enum.

No it's done at our compile time not at users compile time. The output JS just has string literals.

The following very simple typescript can be utilized by both ts and js

// index.ts

const enum A {
  X = "some string"
}

function fn() {
  return A.X;
}

fn();

// index.js
function fn() {
    return "some string" /* X */;
}
fn();

// index.d.ts
declare const enum A {
    X = "some string"
}
declare function fn(): A;

[pauldraper]

I suppose, yeah, JS consumers just need written documentation on what the runtime values are. (Still gonna call this a TS misfeature though.)

export const enum A { ​X, Y }

export function f(a: A) {
  // ...
}

TS consumer:

import { f, A } from 'lib';
f(A.X);

JS consumer:

import { f } from 'lib';
f(0); // because I read docs on runtime value

[Flarna]

I think const enums are fine for internal use but quite useless / error prone if used in exports of a module.

[dyladan]

I think we are all in agreement.

[MSNev]

I don't believe that we are all in agreement.

Const enums are not a mis-feature or useless / error prone for exported modules. If you have a JS user they can pass anything they like so the "error prone" nature exists for all API's.

I've not had cycles recently to look at this, but it's also possible that this can be done after 1.0.0 without breaking anything, which is why I've not been pushing this to date.... But closing based on the comments here I believe is the wrong approach.

[dyladan]

Happy to reopen if there is dissent :)