Impossible to bundle with Rollup: GraphQL should not be a devDependency

[loderunner]

Describe the bug

When trying to bundle a node project that imports dd-trace with Rollup, I get the following errors:

(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
graphql/utilities (imported by graphql/utilities?commonjs-external)
graphql/language/visitor (imported by graphql/language/visitor?commonjs-external)
graphql/language/printer (imported by graphql/language/printer?commonjs-external)

The errors stem from Rollup being impossible to resolve the graphql dependency, as it is declared a devDependency even though it is used in the graphql plugin code.

Environment

• Operation system: macOS 11.4
• Node version: 16.3
• Tracer version: 0.35.0
• Agent version: N/A

[rochdev]

The errors stem from Rollup being impossible to resolve the graphql dependency, as it is declared a devDependency even though it is used in the graphql plugin code.

It's used in the plugin code since the plugin will only be used when graphql is used in the first place, so when that happens it will always be available without the tracer having an explicit dependency on it. Since most bundlers don't support conditional requires, the only way to fix this right now is to ignore the dependency somehow in your bundler configuration. For example, in Webpack you could set graphql as an external dependency which will remove the error. Not sure how to do the same in Rollup but an equivalent configuration should fix the issue.

[loderunner]

For example, in Webpack you could set graphql as an external dependency which will remove the error. Not sure how to do the same in Rollup but an equivalent configuration should fix the issue.

I already used this workaround for now.

It just seemed like an issue to me that you production code relied on a development-time dependency. Perhaps declaring it in peerDependencies would be closer to your description?

That way I'll get a peer dependency warning from npm or yarn and I know that dd-trace will be the responsible package, without having to do a full-text search in node_modules.

[rochdev]

The problem is that we don't actually depend on it, we just use it if it's there because the plugin was needed which guarantees that it's available, so there shouldn't otherwise be any warning. In that sense, it's not a peer dependency nor an optional dependency or any type of dependency, because the tracer doesn't actually need it to be there unless it's there (if that makes sense). We're going to rework the plugin system soon which will hopefully fix some issues with bundlers, but in the meantime ignoring the module is the only fix.

[toddbascombe]

it seems like esbuild is failing because graphql is a dev dependency in dd-trace but is a external dependency for our service.

[toddbascombe]

will there be a fix for this soon?

[rochdev]

@toddbascombe We plan to improve support for bundlers, but I don't have a timeline. In the meantime, it should be enough to put these specific modules as externals in your configuration as described in https://github.com/DataDog/dd-trace-js/issues/827#issuecomment-901218713 which is for Webpack but ESBuild should have a similar options. Alternatively, you could put all of node_modules as externals with something like esbuild-node-externals which would avoid all issues related to bundling dependencies.

[rvalzy]

The problem is that we don't actually depend on it, we just use it if it's there because the plugin was needed which guarantees that it's available

Would it make sense to declare graphql as a peer dependency instead of a dev or non-dev dependency? (Ideally, with a forgiving version range.)

For example, Apollo Server 4.x declares a peer dependency on graphql >= 16.6.0, and sometimes when other packages use different versions of graphql, errors like this get thrown at runtime:

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

Duplicate "graphql" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.

[dobeerman]

Hi all, any updates so far? I'm still experiencing the issue with bundling the app by esbuild@0.

0.738 MISSING: Unable to find "graphql/language/visitor". Is the package dead code?
0.739 ✘ [ERROR] Could not resolve "graphql/language/visitor"
0.739 
0.739     node_modules/dd-trace/packages/datadog-plugin-graphql/src/tools/transforms.js:9:26:
0.739       9 │ const visitor_1 = require("graphql/language/visitor");
0.739         ╵                           ~~~~~~~~~~~~~~~~~~~~~~~~~~

[tlhunter]

@dobeerman regarding esbuild bundling it should be remedied by adding this config:

external: [
  'graphql/language/visitor',
  'graphql/language/printer',
  'graphql/utilities',
],