fix: remove require usages in esm build

[legendecas]

Which problem is this PR solving?

• Fixes Reference of require in esm build of base sdk packages #2464

Short description of the changes

• Enabled esModuleInterop flag in tsconfig.base.json.
• Removes require usages in esm build.
• Also fixes that assuming esmodule namespace object as callable in tests.
• Notably, sinon.stub cannot be used with esmodule namespace objects as fields of namespace objects are readonly. Instead, we should stub on the modules' default exported object, like

import http from 'http';
sinon.stub(http, 'request'); 
// modifications on variable `http` can be reflected to 
// either `import * as http from 'http` or 
// `import { request } from 'http'` in other modules by 
// "live binding" semantics.

• sinon.useFakeTimers and sinon.clock cannot be used with star import anymore as sinon.clock was defined after sinon.useFakeTimers() but not statically

import * as sinon from 'sinon'; // namespace object was sealed with exported names.
sinon.useFakeTimers(); // installs `sinon.clock`, not installed to the namespace object
sinon.clock // => undefined


import sinon from 'sinon'; // default exported object is identical to the exported one.
sinon.useFakeTimer(); // installs `sinon.clock`
sinon.clock // => [SinonFakeClock]

[codecov]

Codecov Report

Merging #2467 (44857fd) into main (07b19ad) will decrease coverage by 0.41%.
The diff coverage is 100.00%.

❗ Current head 44857fd differs from pull request most recent head a174294. Consider uploading reports for the commit a174294 to get more accurate results

@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
- Coverage   92.62%   92.20%   -0.42%     
==========================================
  Files         137      132       -5     
  Lines        5017     4632     -385     
  Branches     1060      989      -71     
==========================================
- Hits         4647     4271     -376     
+ Misses        370      361       -9     

Impacted Files	Coverage Δ
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts	94.49% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️
...ckages/opentelemetry-sdk-metrics-base/src/Meter.ts	88.59% <0.00%> (ø)
...pentelemetry-sdk-metrics-base/src/MeterProvider.ts	88.57% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
...ation-xml-http-request/src/enums/AttributeNames.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts

[meteorlxy]

In addition to this comment #2464 (comment), esModuleInterop is casuing some inconsistent usage in this PR.

For example:

[legendecas]

@meteorlxy That use pattern is explained here #2467 (comment). Of course, we can change all of these namespace imports to default imports. However, in this PR for now I just changed the imports that didn't work with esModuleInterop enabled. So that the PR could be kept small to work.

I don't have a strong opinion to enforce the default import pattern. If people are fine with it, we can do it.

[Flarna]

Is there any chance to add a test for ESM executed on each PR?
Or is setting the esModuleInterop flag enough to ensure that it works for ESM users?

[legendecas]

Is there any chance to add a test for ESM executed on each PR?

As the matter of fact, Node.js native ESM support requires module specifiers to have extensions like import './foo.js'. This invalidates the current esm build products in the repo for Node.js native ESM support. And the ESM loaders API that ts-node depends on is not stable yet, and subject to significant changes (TypeStrong/ts-node#1007).

However, OTEL packages currently work with Node.js modules, interpreted as CJS modules. as Node.js automatically bridges CJS modules to ESM. The "module" field in package.json (https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-core/package.json#L6) is not interpreted by Node.js. Node.js only understands "name", "main", "type", "exports", and "imports" (https://nodejs.org/api/packages.html#packages_node_js_package_json_field_definitions).

The current esm build product is originally generated for transpilers like webpack and rollup to tree shaking unused code pieces (#2112). So what I can tell by now is that maybe we can add a karma configuration to test with esm.

Or is setting the esModuleInterop flag enough to ensure that it works for ESM users?

Setting "esModuleInterop" ensures generated js code importing behavior complies with the ESM standard (except features like direct references to require and import.meta). Running their CJS tests should be sufficient.

[dyladan]

@meteorlxy can you provide an example of what you mean by "contagious" behavior? I wasn't aware that enabling esmoduleinterop put additional requirements on dependent packages.

[meteorlxy]

@legendecas @dyladan Sure. Just made a demo repo: https://github.com/meteorlxy/esmoduleinterop-issue

[dyladan]

Ah I see. The way we are imported doesn't change, but the libcheck fails because it traverses up dependencies.

[meteorlxy]

That's it. So I personally recommend never enabling it in library projects.

[legendecas]

I can share @meteorlxy's sentiments about enabling esModuleInterop flag. However, as far as I can tell, the "contagious" problem only happens in the generated d.ts typing files. That is to say, if we don't export or reference the types that come from non-esm friendly packages, all things are going to be fine with or without esModuleInterop enabled in the dependent consumer packages.

Currently, in published code, the only package that is not compatible is lodash.merge, and other packages like nock and assert are merely used in tests. Also, notably, we do have compatibility tests that have esModuleInterop disabled, in https://github.com/open-telemetry/opentelemetry-js/blob/main/backwards-compatability/node12/tsconfig.json and 3 other similar for node8 and node10. So I'm not quite concerned about enabling esModuleInterop would result in force consumers to enable that option too.

[legendecas]

Or if people are preferring not aggressive with strict esm compliant, and with push-backs from Node.js esm behavior disalignment even when esModuleInterop enabled (microsoft/TypeScript#41139 (comment)), I'd be willing to step back to just replace lodash.merge with another esm-friendly package.

[dyladan]

The compatibility tests use the base tsconfig so they will also inherit the esmoduleinterop flag.

@legendecas it sounds like you think there would still be issues if we just chose a different esm-friendly alternative for lodash.merge?

Is there a way we can enforce in CI that our packages are truly esm friendly? Perhaps it's time for true integration tests where a fake "customer" project is compiled against each PR.

[legendecas]

@dyladan the compat tests are not using tsconfig.base.json but tsconfig.es5.json so esModuleInterop will not be enabled for them by inheritance, See https://github.com/open-telemetry/opentelemetry-js/blob/main/backwards-compatability/node12/tsconfig.json#L2.

Edit: tsconfig.es5.json does inherit tsconfig.base.json. Well...

[dyladan]

We had a discussion about this at SIG. @obecny is going to copy the functionality from lodash.merge into core for our purposes. In the future though, how do we make sure this isn't going to cause problems? Can we make a test application which compiles against the esm target to ensure it is working properly?

[legendecas]

@dyladan maybe we can add a test harness that doesn't interpret require mixed with import statements like rollup with transformMixedEsModules (https://github.com/rollup/plugins/tree/master/packages/commonjs#transformmixedesmodules) disabled.

[dyladan]

@legendecas sounds like a good idea to me. Would you have time to contribute a test like that?

[legendecas]

@dyladan yeah, I can do it. Closing this PR as there is nothing left to be done.