-
Notifications
You must be signed in to change notification settings - Fork 248
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
chore: Split @apollo/subgraph
package from @apollo/federation
#1058
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, this looks like a very comprehensive migration!
My main comment is that it may actually be trying to do too much, because it's keeping the composition bits in @apollo/federation
and makes sure those are still working. That makes sense if we want to decouple the introduction of @apollo/subgraph
from the Fed 2 release, but it seems like a lot of extra work for something that will be short lived because we want to remove that code as soon as the Fed 2 PR lands (which introduces a separate @apollo/composition
package). For that reason, I also think we should ask @pcmanus to review this, so we don't run into surprise merge conflicts later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty great to me. thanks for working on this! couple questions around reaching into @apollo/subgraph/dist
and if we want to stand by (or adjust based on something else). Wouldn't mind a review from @pcmanus either though just to share some thoughts about where things land.
federationDirectives, | ||
} from '../directives'; | ||
isFederationDirective, | ||
} from '@apollo/subgraph/dist/directives'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to depend on these in this direction right now. Just might be one of those things that doesn't find the perfect home until after @pcmanus finishes working on breaking out some other packages (for composition)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, presumably most or all of this stuff disappears in the future!
Responding to @martijnwalraven's comments:
I think the PR tried to actively avoid conflicts with other work in flight.
We absolutely do and that was an explicit goal of handing this off.
Perhaps, but it's done now, already and happened within a day. 😄 (And functionally speaking, we've just shared and spread out the workload rather than trying to just do everything at once.)
Definitely would support this if @pcmanus has time to afford to this, my quick smoke test says there are no conflicts that couldn't be resolved in a few seconds. (it's like, array ordering of the package members |
I had a relatively quick scan, and this does what I would expect it to do. I'm sure it'll create a handful of merge conflict when merged against the fed2 code, but I suspect those will be trivial so I'm not too worried about it.
Out of curiosity, I do see how it's keeping composition in |
@pcmanus It's mostly about fixing dependencies that (after the split) go the other way, see https://github.com/apollographql/federation/pull/1058/files#r724083941 for example. |
Co-authored-by: Jesse Rosenberger <[email protected]>
This PR separates the concerns of running a subgraph from composition, which the
@apollo/federation
package is currently responsible for both of. The new@apollo/subgraph
package will be used in the same exact manner for subgraph developers.There were some interdependent types, functions, etc. within the federation package which I had to separate to one or the other. Given that the
@apollo/federation
already had an implicit dependency on this new package (for forward-compat), I chose to migrate the few bits into the new package and deeply import them from@apollo/federation
. I'm not particularly interested in exposing all of those common bits to the minimal public API of@apollo/subgraph
, hence the nested import.Other notable updates:
TODO