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.

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

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

Open
SimenB opened this issue Jan 31, 2023 · 9 comments
Open

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

SimenB opened this issue Jan 31, 2023 · 9 comments
Assignees
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc

Comments

@SimenB
Copy link
Contributor

SimenB commented Jan 31, 2023

What happened?

Steps to Reproduce

Import @opentelemetry/exporter-trace-otlp-http@0.35.1 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 SimenB added bug Something isn't working triage labels Jan 31, 2023
@dyladan dyladan self-assigned this Jan 31, 2023
@dyladan dyladan added priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc and removed triage labels Jan 31, 2023
@dyladan
Copy link
Member

dyladan commented Feb 9, 2023

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?

@SimenB
Copy link
Contributor Author

SimenB commented Feb 9, 2023

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.

@dyladan
Copy link
Member

dyladan commented Feb 9, 2023

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

@SimenB
Copy link
Contributor Author

SimenB commented Feb 9, 2023

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

@Flarna
Copy link
Member

Flarna commented Feb 10, 2023

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 #3257 recently regarding this.

found one more: #936

@jschuttler
Copy link

jschuttler commented Jun 7, 2023

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.

@dyladan
Copy link
Member

dyladan commented Jun 21, 2023

@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

@triptec
Copy link

triptec commented Aug 31, 2023

I have this problem and found microsoft/TypeScript#36146 (comment)

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 #3580 (comment)

@david-luna
Copy link
Contributor

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 skipLibCheck: true is ignored when package's types field points to a .ts file (not a .d.ts declaration file) microsoft/TypeScript#41883 (comment)

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 open-telemetry/opentelemetry-js-contrib#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.

luismiramirez added a commit to appsignal/appsignal-nodejs that referenced this issue Jan 31, 2024
luismiramirez added a commit to appsignal/appsignal-nodejs that referenced this issue Jan 31, 2024
luismiramirez added a commit to appsignal/appsignal-nodejs that referenced this issue Jan 31, 2024
luismiramirez added a commit to appsignal/appsignal-nodejs that referenced this issue Jan 31, 2024
luismiramirez added a commit to appsignal/appsignal-nodejs that referenced this issue Jan 31, 2024
luismiramirez added a commit to appsignal/appsignal-nodejs that referenced this issue Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

No branches or pull requests

6 participants