chore: update module config in tsconfig.base.json

[david-luna]

Which problem is this PR solving?

Follow up PR for the TypeScript update. It updates the module compiler option to the value node16 replacing the former value commonJS. According to the docs is the recommended option for libs targeting NodeJS v12+ and it won't change the output emitted by the compiler.

ref: https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext

Closes #5310

Disabling esModuleInterop

The new compiler option enables esModuleInterop ((ref)[https://www.typescriptlang.org/tsconfig/#esModuleInterop]) which makes thew compile more strict about the way you import modules in your files. This makes the type checking fail when a module exports is a function and a namespace at the same time causing errors like

api/test/common/context/NoopContextManager.test.ts:28:9 - error TS2349: This expression is not callable.
  Type 'typeof assert' has no call signatures.

28         assert(
           ~~~~~~

  api/test/common/context/NoopContextManager.test.ts:17:1
    17 import * as assert from 'assert';
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Type originates at this import. A namespace-style import cannot be called or constructed, and will cause a failure at runtime. Consider using a default import or import require here instead.
...

For some modules like assert is possible to just refactor the code since the exported function also contains a reference in the namespace so replacing assert(...) to assert.ok(...) is enough. But there are cases like axios and nock that are doing the same type of export (function and namespace) but without providing the exported function within the namespace.

One possible solution to that is to enable allowSyntheticDefaultImports compiler option ((ref)[https://www.typescriptlang.org/tsconfig/#allowSyntheticDefaultImports]) which emulates a default export for these modules and does not affect the output of the compilation.

My 1st approach is to disable esModuleInterop since I not 100% sure if output is the same and consumers of the packages may run into any issue. I think we should discuss it here and make a decision

cc: @open-telemetry/javascript-maintainers

Short description of the changes

• update compiler options in tsconfig.base.json
• refactor tests

Type of change

• Chore (non-breaking change which fixes an issue)

How Has This Been Tested?

• npm run compile
• npm run test

Checklist:

• Followed the style guidelines of this project
• Unit tests have been updated
• Discuss about using allowSyntheticDefaultImports

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.54%. Comparing base (c00f36e) to head (b6ef082).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5347   +/-   ##
=======================================
  Coverage   94.54%   94.54%           
=======================================
  Files         318      318           
  Lines        8051     8051           
  Branches     1694     1694           
=======================================
  Hits         7612     7612           
  Misses        439      439           

[chancancode]

@david-luna from what I understand:

1. This affects the code generation for our imports
2. The exports in our own modules/packages does not have the issue requiring the interop shim
3. We use external dependencies minimally throughout the SDK

So, this mainly just affects tests right?

From a quick search in the codebase, I found one instance of importing from axios in sampler-jaeger-remote in non-test modules, are there any other instances you know of?

If that's correct, my thoughts would be that we use the combination that:

1. emit the smallest/simplest code for imports across the board for the published modules
2. works for the tests usages, possibly with some minor adjustments

Other than the cases where we import from external legacy packages, since this only affect imports, I don't think it can affect consumers, right? So we can probably just focus on those cases for the compatibility discussion. As far as I can tell (haven't tried running this and diffing the output), your approach seems to satisfy all both of those points, considering the tests are passing.

[david-luna]

So, this mainly just affects tests right?

yes, but there are some places where we have the import * pattern which in combination with esModuleInterop produces the boilerplate code mentioned in the documentation.

Made my search and it turns out there are several places in the sources where the pattern import * appears

rg -g '!test/' "import \*"

packages/sdk-metrics/src/export/MetricReader.ts
17:import * as api from '@opentelemetry/api';

packages/sdk-metrics/src/export/PeriodicExportingMetricReader.ts
17:import * as api from '@opentelemetry/api';

doc/context.md
31:import * as api from "@opentelemetry/api";
51:import * as api from "@opentelemetry/api";
64:import * as api from "@opentelemetry/api";
80:import * as api from "@opentelemetry/api";
102:import * as api from "@opentelemetry/api";
137:import * as api from "@opentelemetry/api";
150:import * as api from "@opentelemetry/api";
165:import * as api from "@opentelemetry/api";
178:import * as api from "@opentelemetry/api";
206:import * as api from "@opentelemetry/api";

packages/sdk-metrics/src/state/MetricStorageRegistry.ts
22:import * as api from '@opentelemetry/api';

packages/sdk-metrics/src/view/Aggregation.ts
17:import * as api from '@opentelemetry/api';

packages/sdk-metrics/src/aggregator/exponential-histogram/mapping/ExponentMapping.ts
16:import * as ieee754 from './ieee754';
17:import * as util from '../util';

api/src/context/NoopContextManager.ts
18:import * as types from './types';

packages/sdk-metrics/src/aggregator/exponential-histogram/mapping/LogarithmMapping.ts
16:import * as ieee754 from './ieee754';
17:import * as util from '../util';

experimental/packages/shim-opencensus/src/ShimTracer.ts
17:import * as oc from '@opencensus/core';

packages/opentelemetry-shim-opentracing/src/shim.ts
17:import * as api from '@opentelemetry/api';
24:import * as opentracing from 'opentracing';

experimental/packages/shim-opencensus/src/shim.ts
19:import * as oc from '@opencensus/core';

experimental/packages/shim-opencensus/src/metric-transform.ts
17:import * as oc from '@opencensus/core';

experimental/packages/shim-opencensus/src/ShimSpan.ts
17:import * as oc from '@opencensus/core';

experimental/packages/shim-opencensus/src/OpenCensusMetricProducer.ts
18:import * as oc from '@opencensus/core';

experimental/packages/shim-opencensus/src/trace-transform.ts
17:import * as oc from '@opencensus/core';

experimental/packages/shim-opencensus/src/propagation.ts
17:import * as oc from '@opencensus/core';

packages/opentelemetry-exporter-jaeger/src/jaeger.ts
28:import * as jaegerTypes from './types';

experimental/packages/otlp-exporter-base/src/transport/http-transport-utils.ts
16:import * as http from 'http';
17:import * as https from 'https';
18:import * as zlib from 'zlib';

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
17:import * as api from '@opentelemetry/api';
24:import * as core from '@opentelemetry/core';
25:import * as web from '@opentelemetry/sdk-trace-web';

experimental/packages/sdk-events/src/EventLogger.ts
18:import * as api from '@opentelemetry/api-events';

experimental/packages/sdk-events/src/EventLoggerProvider.ts
16:import * as api from '@opentelemetry/api-events';

experimental/packages/opentelemetry-instrumentation-fetch/src/types.ts
17:import * as api from '@opentelemetry/api';

packages/opentelemetry-core/src/common/time.ts
17:import * as api from '@opentelemetry/api';

experimental/packages/otlp-transformer/src/logs/protobuf/logs.ts
16:import * as root from '../../generated/root';

experimental/packages/opentelemetry-instrumentation-fetch/src/utils.ts
20:import * as api from '@opentelemetry/api';

experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts
29:import * as shimmer from 'shimmer';

packages/opentelemetry-exporter-zipkin/src/zipkin.ts
21:import * as zipkinTypes from './types';

experimental/packages/otlp-transformer/src/common/protobuf/protobuf-export-type.ts
17:import * as protobuf from 'protobufjs';

experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts
19:import * as path from 'path';

experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
17:import * as types from '../../types';
18:import * as path from 'path';

packages/opentelemetry-exporter-zipkin/src/platform/node/util.ts
19:import * as http from 'http';
20:import * as https from 'https';
21:import * as url from 'url';
22:import * as zipkinTypes from '../../types';

packages/opentelemetry-context-async-hooks/src/AsyncHooksContextManager.ts
18:import * as asyncHooks from 'async_hooks';

experimental/packages/otlp-transformer/src/trace/protobuf/trace.ts
17:import * as root from '../../generated/root';

experimental/packages/opentelemetry-instrumentation-xml-http-request/src/types.ts
17:import * as api from '@opentelemetry/api';

packages/opentelemetry-exporter-zipkin/src/platform/browser/util.ts
23:import * as zipkinTypes from '../../types';

experimental/packages/opentelemetry-instrumentation-xml-http-request/src/utils.ts
20:import * as api from '@opentelemetry/api';

packages/opentelemetry-exporter-zipkin/src/transform.ts
17:import * as api from '@opentelemetry/api';
20:import * as zipkinTypes from './types';

experimental/packages/opentelemetry-instrumentation/src/platform/browser/instrumentation.ts
18:import * as types from '../../types';

experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
17:import * as api from '@opentelemetry/api';

experimental/packages/otlp-transformer/src/metrics/protobuf/metrics.ts
17:import * as root from '../../generated/root';

packages/opentelemetry-resources/src/detectors/platform/node/ProcessDetectorSync.ts
33:import * as os from 'os';

packages/opentelemetry-core/src/trace/TraceState.ts
17:import * as api from '@opentelemetry/api';

packages/opentelemetry-sdk-trace-web/src/utils.ts
23:import * as api from '@opentelemetry/api';

packages/opentelemetry-resources/src/detectors/platform/node/machine-id/execAsync.ts
17:import * as child_process from 'child_process';
18:import * as util from 'util';

packages/opentelemetry-resources/src/detectors/platform/node/machine-id/getMachineId-win.ts
17:import * as process from 'process';

packages/opentelemetry-resources/src/detectors/platform/node/machine-id/getMachineId.ts
16:import * as process from 'process';

experimental/packages/sdk-logs/src/LogRecord.ts
19:import * as api from '@opentelemetry/api';

experimental/packages/otlp-grpc-exporter-base/src/create-service-client-constructor.ts
17:import * as grpc from '@grpc/grpc-js';

experimental/packages/sampler-jaeger-remote/src/JaegerRemoteSampler.ts
24:import * as axios from 'axios';

packages/opentelemetry-sdk-trace-base/src/Tracer.ts
17:import * as api from '@opentelemetry/api';

experimental/packages/otlp-grpc-exporter-base/src/configuration/otlp-grpc-env-configuration.ts
25:import * as fs from 'fs';
26:import * as path from 'path';

experimental/packages/opentelemetry-instrumentation-http/src/utils.ts
73:import * as url from 'url';

experimental/packages/opentelemetry-instrumentation-http/src/internal-types.ts
20:import * as url from 'url';

Compiling with esModuleInterop adds the TS helper functions at the top for each file that has the import * pattern

var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    var desc = Object.getOwnPropertyDescriptor(m, k);
    if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) {
      desc = { enumerable: true, get: function() { return m[k]; } };
    }
    Object.defineProperty(o, k2, desc);
}) : (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    o[k2] = m[k];
}));
var __setModuleDefault = (this && this.__setModuleDefault) || (Object.create ? (function(o, v) {
    Object.defineProperty(o, "default", { enumerable: true, value: v });
}) : function(o, v) {
    o["default"] = v;
});
var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (k !== "default" && Object.prototype.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k);
    __setModuleDefault(result, mod);
    return result;
};

IMHO having this in many places (even several times in the same package) is not desirable. So to me enabling this option means to set a convention of not using import * in any package sources. This convention would be there to:

• avoid to emit different code. Although I also think it won't affect consumers.
• avoid package size increase. Specially for web packages

NOTE: if we enable it I think we can leave import * in test code since it wont affect the tests

[chancancode]

Hey @david-luna, sorry I buried the lead – to be clear, I was supportive of your conclusion to turn off esModuleInterop. It seems like a good path forward to me because:

1. It's desirable to not inflate code size by emitting the interop shim
2. Predominately we are importing from our own modules, which does not require the inter-op shim
3. Going forward, the ecosystem is switching to shipping ESM (in addition to CJS at the very least), so it's increasingly less likely that new code with introduce the problem requiring the shim
4. So mostly this interop issue only arise when interfacing with external legacy packages/modules, which occurs sparingly outside of tests (since we have minimal external dependencies)
5. Other than those limited legacy cases (which we will shed over time), this still allows us to freely make full use of all the valid modern patterns, including import * when that is appropriate

I'm +1 disabling esModuleInterop, assuming we can make it work. The code changes in here are also an improvement to my eyes regardless. Since I'm in favor of your proposal, I was focusing on whether we can make it work and understanding the state of this PR.

I'm assuming, because the tests are passing, any potential issues within the tests are already taken care of and things appear to be working just fine after your changes.

So, from your original comment, and from my logic above, I was inferring that the only places left to pay attention to are imports from things like axios and the like outside of tests (which possibly isn't covered by tests?), is that right?

[trentm]

[Most of the top of this comment was already covered in discussion above, which I lamely did not first read. tl;dr from me: setting esModuleInterop: false sounds good to me.]

But there are cases like axios and nock that are doing the same type of export (function and namespace) but without providing the exported function within the namespace.

I played just in packages/opentelemetry-exporter-zipkin, which is using nock. I made this change:

diff --git a/packages/opentelemetry-exporter-zipkin/test/node/zipkin.test.ts b/packages/opentelemetry-exporter-zipkin/test/node/zipkin.test.ts
index c36722f89..55cb40af8 100644
--- a/packages/opentelemetry-exporter-zipkin/test/node/zipkin.test.ts
+++ b/packages/opentelemetry-exporter-zipkin/test/node/zipkin.test.ts
@@ -15,7 +15,7 @@
  */

 import * as assert from 'assert';
-import * as nock from 'nock';
+import nock from 'nock';
 import { ReadableSpan } from '@opentelemetry/sdk-trace-base';
 import {
   ExportResult,
diff --git a/tsconfig.base.json b/tsconfig.base.json
index fd9a19175..e65e1a467 100644
--- a/tsconfig.base.json
+++ b/tsconfig.base.json
@@ -10,7 +10,7 @@
     "inlineSources": true,
     "module": "node16",
     "moduleResolution": "node16",
-    "esModuleInterop": false,
+    "esModuleInterop": true,
     "newLine": "LF",
     "noEmitOnError": true,
     "noFallthroughCasesInSwitch": true,

THis compiles and tests pass.
The change in the emitted build/test/node/zipkin.test.js is here: https://gist.github.com/trentm/b537c228a4e745ea99d5d464bf4507c4

So with esModuleInterop: true we are getting a lot of __importDefault and __importStar boilerplate from TypeScript for that interop. It may be that this only happens for few, if any modules in OTel JS runtime code? I'm not sure.

I think setting esModuleInterop: false for now is fine, given that it is the current behaviour before this PR. Changing to esModuleInterop: true could be discussed separately.

[trentm]

Merge conflict to deal with, but otherwise LGTM.

[pichlermarc]

Looks good - i think disabling esModuleInterop is the correct approach. Turning it on may require consumers of our packages also to turn it on.

[pichlermarc]

We should also consider (in a different PR) bumping the ESM target to ES2020 so that it is already updated when we release SDK 2.0 (as a follow-up to our support policy change from #5059).