opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

`@opentelemetry/otlp-exporter-base` TS error in Node environment

Open SimenB opened this issue 2 years ago • 9 comments

What happened?

Steps to Reproduce

Import @opentelemetry/[email protected] without dom types available

Expected Result

No TypeScript error

Actual Result

../../node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/browser/util.d.ts(10,84): error TS2304: Cannot find name 'BlobPropertyBag'.
../../node_modules/@opentelemetry/otlp-exporter-base/build/src/platform/browser/util.d.ts(20,52): error TS2304: Cannot find name 'Blob'.

Additional Details

Regressed in #3208

OpenTelemetry Setup Code

import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http';

package.json

No response

Relevant log output

No response

SimenB avatar Jan 31 '23 14:01 SimenB

I'm surprised this hasn't come up before. I don't think there is any way for us to bundle the types for the browser-specific files. I'm wondering how our compiler passes since I don't see the "DOM" lib included in any tsconfig. Am I missing some obvious configuration that allows us to compile? Where is it referencing those types from?

dyladan avatar Feb 09 '23 18:02 dyladan

We've had this issue in Jest as well. If some dependency, somewhere in the monorepo, has a /// references lib="dom" (or whatever the syntax is) somewhere, or its @types/* does, it "pollutes" the entire tree with the globals. So you don't get errors in your own project, but consumers of your modules do.

SimenB avatar Feb 09 '23 19:02 SimenB

How did you solve it? I'm shocked this hasn't come up before since there are browser types in other places as well

dyladan avatar Feb 09 '23 19:02 dyladan

In our case it was @types/jsdom which pulled in dom. Solution was to make jsdom optional 😅 AFAIK there's not a more general solution

SimenB avatar Feb 09 '23 19:02 SimenB

The tsconfig file used here doesn't specify which lib to use. As a result the ts defaults are used based on target and this includes dom.

I created https://github.com/open-telemetry/opentelemetry-js/issues/3257 recently regarding this.

found one more: https://github.com/open-telemetry/opentelemetry-js/issues/936

Flarna avatar Feb 10 '23 07:02 Flarna

Any updates on this? The only way I've been able to work around it is by adding the dom entry to libs in tsconfig. I checked out #3257 and #936 but haven't seen any updates there either.

jschuttler avatar Jun 07 '23 13:06 jschuttler

@jschuttler no real update. I tried a bit today to strip the dom types out of our packages (e.g. by using any or unknown or @ts-ignore directives) and it works but it massively compromises the safety of the types. Maybe that's ok?

One possible solution would be to publish our own package that exports a typed global object and use it like import { performance } from '@opentelemetry/typed-global'; or similar

dyladan avatar Jun 21 '23 18:06 dyladan

I have this problem and found https://github.com/microsoft/TypeScript/issues/36146#issuecomment-1190925432

So I've added src/override.d.ts with:

export {};
declare global {
  type BlobPropertyBag = unknown
}

as BlobPropertyBag was my problem.

I guess that's in the same vein as https://github.com/open-telemetry/opentelemetry-js/issues/3580#issuecomment-1601330597

triptec avatar Aug 31 '23 14:08 triptec

Hi,

I've had a similar issue in a project and we came with a couple of ways to avoid this error

  1. add DOM to the lib compiler option https://www.typescriptlang.org/tsconfig#lib to the project's tsconfig.json. You get the types from the browser although you're compiling a project for node platform but makes the compiler happy
  2. a more overkill solution is to enable skipLibCheck in the compiler options. This will disable type checking on any declaratin file of your dependencies but maybe you don't want that. Also this option does have issues depending on the package contents https://github.com/microsoft/TypeScript/issues/41883#issuecomment-1758692340

IMHO having sources for both platforms (node/browser) is very challenging regarding types. Sooner or later you will reference them and you'll get type errors in one platform or the other.

I don't think there is any way for us to bundle the types for the browser-specific files.

If if got it right TypeScript v4.5 allows to specify the types explicitly as a dependency https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#supporting-lib-from-node_modules. This may help us to avoid this issue since types will be installed and pinned to a version. The possible drawback is that we may land in another type of issues like the one we had with express https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1787

Also we cannot forget that the problem can happen both ways. A project for web platform that compilations fails because of missing node types and then we will also try to strip node types from the sources.

One possible solution would be to publish our own package that exports a typed global object and use it like import { performance } from '@opentelemetry/typed-global'; or similar

would that package then contain a mix of browser and node types? if we are unlucky and there is a type with many other types embedded should we need to copy all? if not at which level do we stop?

We can think for now of adding these types into the package itself. The type BlobPropertyBag is easily portable and could be added into the types file but Blob maybe not so easy (was added to node in v18 and we are supporting >=v14)

I'm wondering if node/browser code needs to be in the same package. I guess this was already discussed so it would be good to have a like to the discussion if there is one.

Sorry, I think I'm adding more questions instead contributing to a solution.

david-luna avatar Jan 24 '24 19:01 david-luna