Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: update module config in tsconfig.base.json #5347

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

chore: update module config in tsconfig.base.json #5347

wants to merge 4 commits into from

Conversation

david-luna
Copy link
Contributor

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

@david-luna david-luna requested a review from a team as a code owner January 15, 2025 16:58
@codecov Codecov
Copy link

codecov bot commented Jan 15, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.58%. Comparing base (fc0edd8) to head (550a09d).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #5347   +/-   ##
=======================================
  Coverage   94.58%   94.58%           
=======================================
  Files         318      318           
  Lines        8069     8069           
  Branches     1701     1701           
=======================================
  Hits         7632     7632           
  Misses        437      437

@chancancode
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

set module compiler option to node16 in tsconfig.base.json
2 participants
@david-luna @chancancode