@opentelemetry/instrumentation-graphql Version 0.27.1 Missing @types/graphql dependency

[asafpelegcodes]

What version of OpenTelemetry are you using?

1.0.4

What version of Node are you using?

v16.11.0

Please provide the code you used to setup the OpenTelemetry SDK

import { GraphQLInstrumentation } from "@opentelemetry/instrumentation-graphql";

What did you do?

In a fresh folder

yarn add @opentelemetry/api 
yarn add @opentelemetry/instrumentation-graphql
yarn add -D typescript

Add the build script to your package.json

{
  "scripts": {
    "build": "tsc otel.ts"
  },
  "devDependencies": {
    "typescript": "^4.5.4"
  },
  "dependencies": {
    "@opentelemetry/api": "^1.0.4",
    "@opentelemetry/instrumentation-graphql": "^0.27.1"
  }
}

Attempt to compile the typescript

yarn build

What did you expect to see?

I expected the file to compile successfully

What did you see instead?

The file failed to compile because the graphql dependency was not installed

$ tsc otel.ts
node_modules/@opentelemetry/instrumentation-graphql/build/src/instrumentation.d.ts:2:36 - error TS2307: Cannot find module 'graphql' or its corresponding type declarations.

2 import type * as graphqlTypes from 'graphql';

Additional context

In Version 0.26.1 it contains the dependency "@types/graphql": "14.5.0" which has somehow been dropped in the latest version

[vmarchaud]

It has been dropped in this PR #754, we need to find a long term solution for this
cc @nata7che

[Flarna]

Please note that @types/graphql is deprecated because graphql itself contains the types now. Therefore it would be needed to add graphql itself as dependency which most likely results in two different graphql versions installed.

Other instrumentations may have similar issues.

As far as I remember the easiest workaround is to set skipLibCheck: true in your tsconfig file.

[dyladan]

As a long-term solution that doesn't require our users to skip lib check we could hand-roll our own types (or copy official ones)

[asafpelegcodes]

Isn't copying the types more error prone than simply making graphql a dependency?

That being said I'm not sure what the best solution is but definitely the workaround suggested is not a great answer.

Since the graphql instrumentation is one of the requirements for regular node auto-instrumentation package I do hope a fix for this gets bumped up since it's affecting users who have no interest in the graphql instrumentation itself.

[dyladan]

Isn't copying the types more error prone than simply making graphql a dependency?

yes, but taking graphql as a dependency is also not ideal, see #746. tests that run against multiple versions might be a mitigating factor in this.

That being said I'm not sure what the best solution is but definitely the workaround suggested is not a great answer.

It may not be the long-term solution, but that's why its called a work around :)

It seems to me that there are only two workable solutions:

1. check graphql types directly into our repository. possibly this process can be automated in some way
2. do not use types for the graphql module. supporting multiple versions already means that the types are only accurate for one version tested anyway

[asafpelegcodes]

@dyladan thanks for the prompt response and just be clear I appreciate what's going on here and I didn't intend to cast a negative spin on the conversation.

I understand the workaround is temporary but I should have been more articulate and said that it makes me nervous to introduce impactful changes into a libraries typescript configuration since A) they can are poorly understood and B) they can easily be overlooked in the future.

Anyway, I'm glad you have a clear visibility on a long term solution because I can see this compilation error biting anyone who pulls down @opentelemetry/auto-instrumentations-node

[dyladan]

Code owner for this module is listed as @obecny but I'm not sure if he is still planning on maintaining it since he stepped down from the core maintainer position.

@obecny please respond here ASAP to let us know if you want to handle this or if one of us should do it.

[asafpelegcodes]

FYI installing the deprecated package @types/graphql as a dev dependency is also a workaround

[nata7che]

@asafpelegcodes
That's actually what caused the version issue in the first place and why the fix was introduced. Or did you mean add an earlier version? (earlier to the problematic one i.e. 14.2.3)

[dyladan]

@asafpelegcodes That's actually what caused the version issue in the first place and why the fix was introduced. Or did you mean add an earlier version? (earlier to the problematic one)

Actually the version issue was caused because we had graphql as a dependency, not @types/graphql. We originally depended on @types/graphql but switched to depending on graphql because it started bundling its own types and the @types/graphql package was deprecated.

[Flarna]

We had @types/graphql version 14.5.0 as dependency. This version is already a dummy which has only "graphql": "*" as a transitive dependency.

I was assuming that version * should not result in an additional graphql version installed. In case user app doesn't use the fallback is latest. But maybe the various package managers (npm, yarn,...) in all their versions handle this different.

Maybe incremental adding dependencies causes also problems (e.g. install first @opentelemetry/instrumentation-graphql which installs latest graphql as dependency and later add a different version of graphql).

[dyladan]

We had @types/graphql version 14.5.0 as dependency. This version is already a dummy which has only "graphql": "*" as a transitive dependency.

I had forgotten that detail

I was assuming that version * should not result in an additional graphql version installed. In case user app doesn't use the fallback is latest. But maybe the various package managers (npm, yarn,...) in all their versions handle this different.
Maybe incremental adding dependencies causes also problems (e.g. install first @opentelemetry/instrumentation-graphql which installs latest graphql as dependency and later add a different version of graphql).

In general, I think this deduplication is usually pretty good, but it might be possible to end up with a scenario like this:

node_modules/graphql@X.y.z
node_modules/@otel/plugin-graphql@x.y
node_modules/@otel/plugin-graphql/node_modules/graphql@Y.y.z

I'm not sure if this would cause problems or not though even if it happened.

[nata7che]

The problem we experienced was caused because the latest graphql version (16) was installed and specifically this version dropped support of node 10 (and was actually even breaking for node 12.18.1).

[Flarna]

@nata7che Can you remember what you selected in your package.json? Did you use npm, yarn or something else?

[nata7che]

@Flarna yep, it was yarn but I assume the same behavior would also occur with npm?

[dyladan]

Yarn actually checks the engines configuration in your package.json and the package.json of all your dependencies and will fail to install any dependency which does not support your current node version. At least as far as I'm aware, npm does not do this check.

[shaicantor]

So what is the recommended workaround at the moment?

[dyladan]

Current workaround is probably to do --skipLibCheck. Unassigning @obecny because he is no longer maintainer and hasn't responded on this issue.

The long-term solution will probably be to vendor the types in-package.

[asafpelegcodes]

@shaicantor on my project we decided to go with installing @types/graphql as a dev dependency. The library is deprecated but I was more comfortable with that choice since my code will break once it's no longer available versus the --skipLibCheck which in my estimation would be harder to remember to revert once the issue is resolved.