`export const enum SemconvStability` from `@opentelemetry/instrumentation` is problematic

[trentm]

We recently exported a const enum: export const enum SemconvStability from #5684, released in @opentelemetry/instrumentation@0.201.0. From a quick look, there are no other const enum exports from packages in this repo.

TIL exporting a const enum from a package is problematic for some usages of the package. https://ncjamieson.com/dont-export-const-enums/ explains the issue. Some tooling will compile TS modules one file at a time in isolation. In those cases, a const enum reference, e.g. SemconvStability.OLD, cannot be resolved. One such tooling example is ts-node being used by mocha for testing in opentelemetry-js.git and opentelemetry-js-contrib.git. I gather that this was not an issue for usage of SemconvStability in the same repo (e.g. in instr-http, instr-xhr, instr-fetch), because it is a monorepo where the dependencies are symlinks to the source dir.

We hit the error in this PR, the first one in opentelemetry-js-contrib to attempt to use SemconvStability: open-telemetry/opentelemetry-js-contrib#2671 (comment)

To fix this:

• Change the SemconvStabilty from const enum to enum. I've confirmed this fixes the testing issue for the instrumentation-knex PR above. fix(instrumentation): change SemconvStability export from const enum to enum #5692
• Do a patch release to fix this. (Marc did a v0.201.1 release in chore: prepare next release #5696)
• Discuss whether we want to add isolatedModules: true to our tsconfig.json in the opentelemetry-js repo. This config is suggested in the blog post above as a way to guard against exporting const enums. Adding this config also means dealing with a number (153) of cases of error TS1205: Re-exporting a type when 'isolatedModules' is enabled requires using 'export type'. in our packages, so this is a larger change. (See chore: enable tsconfig isolatedModules #5697)
• See chore: set isolatedModules compilation option opentelemetry-js-contrib#2839 for contrib repo

[cjihrig]

Have you considered migrating toward erasableSyntaxOnly?

[JamieDanielson]

• Discuss whether we want to add isolatedModules: true to our tsconfig.json in the opentelemetry-js repo.

I am a fan of this idea. It sounds similar to the typescript-eslint rule for "consistent-type-imports" that was recently enabled in an internal codebase. This plus the "consistent-type-exports" seems to do about the same thing anyway. Recently came across this explainer called Consistent Type Imports and Exports: Why and How | typescript-eslint that provided some useful explanation too. I haven't looked into how much overlap there is (are these doing the same thing?).

Have you considered migrating toward erasableSyntaxOnly?

erasableSyntaxOnly looks like a good idea too (some details here), though it's only available in TypeScript 5.8 and we're on 5.0.4 🤔 I can't recall why we stayed at that version but if we do upgrade we should probably enable that.

[trentm]

Have you considered migrating toward erasableSyntaxOnly?

(https://www.typescriptlang.org/tsconfig/#erasableSyntaxOnly)

That will be some effort, though I agree it sounds like a good idea.

In the core repo there are 37 cases of "Parameter properties", just on constructor( functions, so likely more:

% rg 'constructor\(' -g '*.ts' | rg '(private|public)' | wc -l
      37

There are fair number of enums, including in the @opentelemetry/api package:

% rg -g '*.ts' -g '!*.d.ts' '\benum\b ' | wc -l
      73

One import ... = require(...), I don't know why:

% rg -g '*.ts' -g '!*.d.ts' 'import [\w_]+ ='
experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
85:import forwardedParse = require('forwarded-parse');

[trentm]

Perhaps verbatimModuleSyntax would be appropriate way to enable isolatedModules and I think a bit more. Introduced in TS "5.0".

... though I'm getting lost in the caveats about CommonJS output.

[legendecas]

verbatimModuleSyntax assumes that there will be minimum translation on the module syntax. So it requires that if the output is in CJS, the source must be written in CJS as well. I don't think it could be enabled if we need to publish both ESM and CJS (which is our current workflow).