`@apollo/gateway` seems to have invalid package.json

[andreialecu]

It appears that @apollo/gateway has a really bizarre way of defining its dependencies:

https://github.com/apollographql/federation/blob/4d5635576f43f36c6b6a14f662edbce328c7aa66/gateway-js/package.json#L27-L42

https://cdn.jsdelivr.net/npm/@apollo/gateway@0.35.1/package.json

Should they be peer dependencies instead?

The problem is that when using yarn, there will be typescript errors after installing @apollo/gateway with apollo-server-core v2 because it brings its own v3 apollo client server along, and the ambient types conflict with each other:

/node_modules/apollo-cache-control/dist/index.d.ts(24,9): error TS2717: Subsequent property declarations must have the same type.  Property 'cacheControl' must be of type 'ResolveInfoCacheControl', but here has type '{ setCacheHint: (hint: CacheHint) => void; cacheHint: CacheHint; }'.

The technical reason for this error is that apollo-cache-control is deprecated in v3, and there's a conflict between the types.

[trevor-scheer]

Hey @andreialecu, can you please explain what you find bizarre and which packages you suggest be peerDependencies? For clarity, I'll assume you mean apollo-server-core and not "v3 apollo client", but let me know if I'm mistaken.

If I create a new project and run the following:

npm i @apollo/gateway
npm i apollo-server-core@2

The gateway package does not bring its own v3 server package if your project brings a compatible server version, so I don't quite follow.

npm ls apollo-server-core
├─┬ @apollo/gateway@0.35.1
│ └── apollo-server-core@2.25.2 deduped
└── apollo-server-core@2.25.2

Can you provide a minimal project where I can reproduce this? Also, this cache control conflict is not unfamiliar. @glasser may understand the specifics of why it happens better than I. Though I think generally speaking, if you were to update all of your apollo dependencies (if that's an option), this issue should resolve itself.

[andreialecu]

Hey @trevor-scheer, thanks for the answer!

That sort of behavior you mention only applies to npm, and is non standard. "Deduping" is not guaranteed and yarn will not dedupe by default. I'm not sure how pnpm deals with it but it's possible there are problems there as well.

Here's how the sequence of commands you mentioned work via yarn:

cd `mktemp -d`
yarn init -y
yarn add apollo-server-core@2 @apollo/gateway

➜ yarn why apollo-server-core
yarn why v1.22.11
[1/4] 🤔  Why do we have the module "apollo-server-core"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "apollo-server-core@2.25.2"
info Has been hoisted to "apollo-server-core"
info This module exists because it's specified in "dependencies".
info Disk size without dependencies: "2.36MB"
info Disk size with unique dependencies: "7.81MB"
info Disk size with transitive dependencies: "33.08MB"
info Number of shared dependencies: 47
=> Found "@apollo/gateway#apollo-server-core@3.1.1"
info This module exists because "@apollo#gateway" depends on it.
info Disk size without dependencies: "1.84MB"
info Disk size with unique dependencies: "6.92MB"
info Disk size with transitive dependencies: "28.14MB"
info Number of shared dependencies: 43
✨  Done in 0.24s.

Notice that two versions of apollo-server-core are being installed.

A range like "apollo-server-core": "^2.23.0 || ^3.0.0" when used in dependencies theoretically just means ^3.0.0.

Additionally, when used with yarn version 3 (yarn modern), in PnP mode (https://yarnpkg.com/features/pnp) - it is guaranteed that @apollo/gateway will have its own dedicated instance of apollo-server-core, instead of sharing the same one the main app is using.

Only peer dependencies should be used to define this sort of behavior.

To be frank, I'm not entirely familiar with @apollo/gateway and I don't directly use it. I only ran into this issue because I needed to install it due to it being a dependency of NestJS here: https://github.com/nestjs/graphql/blob/392a9ae7eb49b86e40668083e9eb71e20f0ad72e/lib/interfaces/gql-gateway-module-options.interface.ts#L1

I started running into typescript issues afterwards because I noticed that @apollo/gateway brought along with it apollo-server-core version 3.0 which is not yet supported by nestjs.

[trevor-scheer]

Interesting - I'm less familiar with Yarn's nuances so I appreciate the additional info.

I would disagree with this statement in theory, but in practice (and in non-npm cases) this seems accurate!

A range like "apollo-server-core": "^2.23.0 || ^3.0.0" when used in dependencies theoretically just means ^3.0.0.

I'll pause and acknowledge my bias / familiarity in this ecosystem is almost entirely limited to npm.

In any case, I think you make a fair argument that this a peer dependency - which in turn carries a bit more explicitness around what it means to "bring your own" and supporting multiple major versions. This happens to be quite a bit nicer in npm@7 now as well.

Thanks for bringing this to our attention!

[glasser]

One challenge in transitioning these to peer deps is that users typically do not depend directly on those sub-packages like apollo-server-errors or apollo-server-core, but rather on apollo-server or apollo-server-express, etc. Will peer deps work properly in this case? (One of the many reasons we would love to eventually move to a single-package @apollo/server but we're not there yet.). @andreialecu can you do some testing to see if npm and yarn work well in the context where your project depends on A and X, A depends on B, and X peer-deps on B?

I think some of these dependencies could possibly be decreased. e.g. we should just replace apollo-server-caching with lru-cache, and we could probably move much of what we get from apollo-server-core into apollo-server-types...

Hopefully Nest can move to AS3 soon!

[andreialecu]

One thing to consider is that dependencies are supposed to mean direct sub-dependencies, which are supposed to be used in isolation by that module alone. Ideally they should be isolated so any other module can't access their internals, but node can't currently enforce that.

The only reason this is currently working with the current setup in @apollo/* is a quirk in node's module resolution algorithm, and the fact that npm runs dedupe by default. As you know, node considers paths that are in the same directory as the same instance.

However, Yarn Modern (version 2+) is attempting to overcome this node limitation by something called Plug'n'Play, and virtual resolutions. (see: https://yarnpkg.com/advanced/lexicon/#virtual-package)

With PnP, the resolution mechanism is changed so that dependencies get properly isolated based on how they're defined, so that everything becomes strict. With it, only Peer dependencies can be shared.

Regarding apollo-server, if subpackages are necessary to be shared across separate packages, then they should be defined as peers everywhere.

Basically "stealing" a dependency from another module without properly declaring it is a problem when using Yarn Modern. And it should also be considered a problem for other package managers as well, as well as a correctness issue.

Here's some more reading about how it can lead to edge cases: https://yarnpkg.com/advanced/rulebook/#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies (click to expand the Why paragraph in order to see the problem in detail)

Note that these edge cases are not limited to yarn. It's possible to run into them using npm as well under certain circumstances.

So it becomes very important to properly define dependencies according to their purpose via either dependencies or peerDependencies. Also optional peer dependencies are possible to declare and are also important for package managers in order to ensure that the final installation has the correct hoisting layout. See: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#peerdependenciesmeta

[andreialecu]

As a TLDR 😆: the only way to correctly define singleton packages is using peerDependencies. See: https://yarnpkg.com/advanced/lexicon/#singleton-package

[andreialecu]

Apollo Server has the same problem:

https://cdn.jsdelivr.net/npm/apollo-server@3.1.2/package.json

  "dependencies": {
    "apollo-server-core": "^3.1.2",
    "apollo-server-express": "^3.1.2",
    "express": "^4.17.1"
  },

I believe these should all be peer dependencies, otherwise the express used by apollo-server is not guaranteed to be a singleton (see my previous two posts) - and it can conflict with whatever the root app is using and lead to weird issues.

Note that yarn has the concept of peer dependencies with defaults (as per https://yarnpkg.com/advanced/rulebook/#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies):

The preferred one is to list the dependency (in Next.js's case, webpack) as both a regular dependency and a peer dependency. Yarn will interpret this pattern as "peer dependency with a default", meaning that your users will be able to take ownership of the Webpack package if they need to, while still giving the package manager the ability to emit a warning if the provided version is incompatible with the one your package expects.

I'm not a npm user so I'm not sure if it has a similar concept. However, these would be perfect candidates for being defined as peers with defaults.

[glasser]

I don't think we're doing any stealing here. @apollo/gateway declares dependencies on all the packages it uses.

My point is that the typical structure is:

• Main program depends on apollo-server and @apollo/gateway
• Both apollo-server and @apollo/gateway need various things include apollo-server-core and apollo-server-types (but the main program doesn't directly import these packages)

I don't understand the peer dependency system well enough to understand if a peer dep from @apollo/gateway on apollo-server-core will do what we expect, when that's only a transitive dependency of the main program, not a direct one. Maybe it will — that's why I was suggesting an experiment.

Re apollo-server: that package is the "batteries-included" package which takes care of importing express, creating the express app, and listening for you. It's supposed to hide express inside it. apollo-server-express (the package which lets you attach ApolloServer to an existing Express app) uses peer dependencies as you'd hope.

[andreialecu]

Re apollo-server: that package is the "batteries-included" package which takes care of importing express, creating the express app, and listening for you. It's supposed to hide express inside it. apollo-server-express (the package which lets you attach ApolloServer to an existing Express app) uses peer dependencies as you'd hope.

I see.

Regarding stealing: @apollo/gateway is indeed "stealing" the singleton apollo-server-core from apollo-server in this case. From the docs at:

https://www.apollographql.com/docs/federation/gateway/

The installation instructions are:

npm install @apollo/gateway apollo-server graphql

Both @apollo/gateway and apollo-server depend on apollo-server-core.

One of them is stealing it from the other in this case, because from what I understand, both packages need the same instance of apollo-server-core (thus it should be a singleton).

The reason this works in practice (with npm) is because npm dedupes it, and as mentioned previously, nodejs treats files in the same path as the same instance (it won't load them twice), thus accidentally treating it as a singleton.

However, this is just an implementation side effect of npm. The correct way to define singletons is through peer dependencies.

Looks like there needs to be some additional engineering effort in order to define the dependency relationship correctly between all involved packages.

PS: As per the example in the Why section in: https://yarnpkg.com/advanced/rulebook/#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies it's likely that even with npm the dedupe won't always work properly here, and could run into edge cases depending on other sets of dependencies being installed.

[glasser]

I mean, it's not stealing because @apollo/gateway does depend on apollo-server-core. I thought stealing was when A depends on B which depends on C and then A requires C?

I agree that peer deps will be better, and I'm just reiterating that I'm not sure if peer deps work effectively when they are "nieces" of the peer-depending package rather than "siblings".

[andreialecu]

I'm not sure if peer deps work effectively when they are "nieces" of the peer-depending package rather than "siblings".

That wouldn't work unfortunately. Peer dependencies only work for child packages to depend on packages defined in the parent package, and share the instance with them.

I think the only correct approach would be for apollo-server to have a peer dependency on apollo-server-core. And then @apollo/gateway would have one too.

That way the contract explicitly defines that apollo-server-core is a singleton and will be shared.

(Any other packages that need to share the same instance/be singletons will need these adjustments too)

There's an *alternative* to this, but it's not pretty. `apollo-server` would need to export the "Core" somehow (like via a `getApolloCore()` function), then `@apollo/gateway` would have a peer dependency on `apollo-server`. It would then have to call `getApolloCore()` and work with whatever that returns. But it seems it would be too dirty.

The user would then need to explicitly install apollo-server-core with apollo-server. However, this may not be a big deal for the current userbase because npm7 installs peers by default.

It should still be noted in the documentation though, because users of other package managers may still need to know which peers need to be installed. (Yarn does not automatically install peers, and they have a strong opinion about keeping it that way.)

[Joshswooft]

Also having the same issue with a NEST JS home project

[andreialecu]

Also having the same issue with a NEST JS home project

You have two options to work around it. Either add "skipLibCheck": true to tsconfig.json or add a dependency to @apollo/gateway version 0.22.0 which is a random old one that I found that was pinned to apollo server version 2.

[glasser]

The idea of having apollo-server-core as a peer dep of all the other packages (which needs to be installed separately) would be a pretty significant change to how Apollo Server is used; if we were going to that trouble, we'd probably prefer to bite the bullet and merge all the packages together into @apollo/server instead. (So we'd have one package that implements the core logic and some of the web framework integrations, with optional peer deps on the web frameworks.)