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

Consider removing now unused parts of @apollo/subgraph #1798

Open
pcmanus opened this issue Apr 28, 2022 · 1 comment
Open

Consider removing now unused parts of @apollo/subgraph #1798

pcmanus opened this issue Apr 28, 2022 · 1 comment
Labels
P4 An issue that should be addressed eventually.

Comments

@pcmanus
Copy link
Contributor

pcmanus commented Apr 28, 2022

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:

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
Copy link
Contributor Author

pcmanus commented May 5, 2022

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.

@cpeacock cpeacock modified the milestones: 2.1.0, 2.2.0 May 24, 2022
@jeffjakub jeffjakub removed this from the 2.2.0 milestone Jan 6, 2023
@korinne korinne added the P4 An issue that should be addressed eventually. label Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 An issue that should be addressed eventually.
Projects
None yet
Development

No branches or pull requests

5 participants