federation icon indicating copy to clipboard operation
federation copied to clipboard

Consider removing now unused parts of `@apollo/subgraph`

Open pcmanus opened this issue 2 years ago • 1 comments

The main method of @apollo/subgraph, the one user mostly care about, is buildSubgraphSchema. In federation 2, that method uses the code from the @apollo/federation-internals to do most of it's work (filling up all the missing federation bits, including adding missing directive definitions, adding the _service and _entities fields, etc...). That is, the client-side code uses the same sources as the server side one, which is probably a good thing.

However, none of the previous scaffolding that was previously used to implement that buildSubgraphSchema has been removed, and this mostly due to time constraints leading to the final release. It slips by if you will.

But that means that @apollo/subgraph has a lot of essentially dead code, which we should consider removing. In particular:

  • the directive definitions from https://github.com/apollographql/federation/blob/main/subgraph-js/src/directives.ts and the types from https://github.com/apollographql/federation/blob/main/subgraph-js/src/types.ts are not being used anymore.
  • nor is quite a few of the schema-helper methods used, like buildSchemaFromSDL
  • the printSubgraphSchema method is also not used and not very useful anymore. Printing the schema outputted by buildSubgraphSchema "normally" should be good for essentially all purposes.

A concern however is that all of this code is essentially exported, so some users could directly rely on it, even if this wasn't the primary intent. So this should probably be taken into account for deciding how we move forward with this issue. At a minimum, we should probably start with a period of deprecation for the code we want to remove.

pcmanus avatar Apr 28 '22 15:04 pcmanus

the printSubgraphSchema method is also not used and not very useful anymore.

Thinking back and with my brain plugged, I'll amend that one part as it is not really true. Namely, printSubgraphSchema input is a GraphQLSchema, but if you print that schema "normally", using printSchema from graphQL-js, then you get an output with no directive applications, but subgraph make important use of directive applications, so that's something printSubgraphSchema does "fix" and it's admittedly valuable. Now, in fed1, printSubgraphSchema was also skipping the federation directive definition definition, and it could be argue that this part is less useful, but it does mean the output of printSubgraphSchema is more readable and closer to what users provide as input to buildSubgraphSchema, so I think there is some genuine value to this.

So tl;dr, printSubgraphSchema is probably worth keeping (and while it's a tad broken at the time of this writing, #1831 will fix that).

The rest of this ticket is stil very much on point though.

pcmanus avatar May 05 '22 15:05 pcmanus