Finish removing old composition code #1208
Conversation
pcmanus
force-pushed
the
finish-removing-old-composition
branch
from
d9d590f
to
539836c
Compare
|
@trevor-scheer, @martijnwalraven: I'd appreciate if you guys could have a quick look at this to see if you something removed here that might actually be of genuine use for federation users and should not be removed. Thinking of |
federation-js/src/index.ts
Outdated
| @@ -1,8 +1,8 @@ | |||
| import { buildSubgraphSchema } from '@apollo/subgraph'; | |||
There was a problem hiding this comment.
If this is all that's left of the exports, should we just remove the package and stop publishing it? Its 2 purposes (subgraph and composition) are both completely owned by separate packages, seems reasonable to remove this altogether going into v2.
There was a problem hiding this comment.
This comment is directed at the whole file, not just the one line.
There was a problem hiding this comment.
seems reasonable to remove this altogether going into v2
I would agree (and double-checking, it doesn't seem the module is used anymore within federation itself after this PR). I think the only concerns is that many people will still have imports on @apollo/federation for buildSubgraphSchema and so the question is whether we want to maintain compatibility for that. But as fed 2 is a major, I'd personally say it's a good time for that (if anything, stopping to publish the package would be a good sign that people should move to use @apollo/subgraph).
The type I felt was maybe imported by users (and that the PR moves directly into federation-js/src/index.ts for that reason) is the ServiceDefinition interface because it's used for local gateway configs. But it would arguably make more sense that this interface is exported by @apollo/gateway so we could make it so.
@abernix Opinions?
There was a problem hiding this comment.
Agree with all of the above! v2 seems like exactly the time to remove deprecated bits like this, especially if it means we can stop publishing a package altogether. Also agree with the ServiceDefinition export finding a new home in @apollo/gateway, that sgtm.
There was a problem hiding this comment.
@trevor-scheer @pcmanus I support this, but it would be worth opening an issue that ensures that we at some point run npm deprecate <pkg> <msg> on this package. Doing that will start outputting the deprecation warnings during installation of the package by package managers and be a good signal to developers to make the (remarkably straightforward) change to the new package name. We might even want to do this quite soon to get the gears in motion?
It's worth calling out the impact this has on tutorials and education content that we and others have produced — and even printed books! Those things can all be updated, but in my mind the sooner we get the deprecation warning up and the longer we keep it working (perhaps by having @apollo/federation depend on a caret semver range that automatically gets minor/patch updates to @apollo/subgraph for some time?). We may even want to communicate this directly to those that are thinking about developer experience / docs / blogs / etc.
There was a problem hiding this comment.
I think deprecating only applies to v2 (which we maybe just shouldn't have ever published given that we're open to killing it). The v0.x gateway continues to use @apollo/federation for its composition capabilities, so I don't think we want to deprecate it (unless we broke out composition as well, which I'm not sure is worth the effort).
There was a problem hiding this comment.
Hmm, I see what you're saying. Perhaps what we might consider is then adding a runtime deprecation warning to the use of @apollo/federation for its buildFederatedSchema / buildSubgraphSchema, rather than at the npm level?
There was a problem hiding this comment.
I think we want to:
npm deprecatev2+ versions of@apollo/federation- Runtime deprecate
buildSubgraphSchema(currently re-exported from@apollo/subgraph)
Currently buildFederatedSchema is already runtime deprecated in @apollo/federation and isn't exported by @apollo/subgraph at all.
|
@pcmanus I finalized the cleanup mostly in the latest commit. Feel free to revert if it's decided that we won't remove it completely for now. |
harmonizer/src/js_types.rs
Outdated
| /// | ||
| /// [`graphql-js']: https://npm.im/graphql | ||
| /// [`GraphQLError`]: https://github.com/graphql/graphql-js/blob/3869211/src/error/GraphQLError.js#L18-L75 | ||
| /// TODO: replace with `composition-js` equivalents? |
There was a problem hiding this comment.
Pushed an additional commit that modifies those (that, for completeness sake, ensuring we ship codes will all errors and documenting them is one of the TODO for federation 2 before GA, but "some" of the errors do get code already and I've link to an example of those).
|
Thanks @trevor-scheer, and this lgtm. I did push a final commit to address the links in |
|
Pinged @abernix to have a look |
There was a problem hiding this comment.
LGTM, with my most notable comment being #1208 (comment) but one comment below about removing the callout in the docs that might be worth preserving.
docs/source/api/apollo-subgraph.mdx
Outdated
| > This method previously existed in the `@apollo/federation` package and was renamed from `buildFederatedSchema` after `@apollo/federation` v0.28.0 (the previous name still works, but it might be removed in a future release). | ||
|
|
There was a problem hiding this comment.
Leaving this here seems like a good thing to maintain for the foreseeable future, particularly since it'll likely be years before everyone actually updates? (We could, however, stop mentioning that it still works.)
There was a problem hiding this comment.
Agreed, probably worth keeping this. Added it back with a minor edit to mention that @apollo/federation is no more.
| @@ -18,7 +18,7 @@ While we intend for a future version of composition to be done natively within | |||
| Rust, this allows us to provide a more stable transition using an already stable | |||
| composition implementation while we work toward something else. | |||
|
|
|||
| [`@apollo/federation`]: https://npm.im/@apollo/federation | |||
| [`@apollo/composition`]: https://npm.im/@apollo/composition | |||
- Export `ServiceDefinition` from `@apollo/gateway` since it's use by public methods. - Remove a few more mentions and links to remove `@apollo/federation`.
pcmanus
force-pushed
the
finish-removing-old-composition
branch
from
130c17a
to
3d27ce1
Compare
|
Sorry for the slow turnaround. Rebased and committed and created #1228 to follow-up on the npm deprecation of I will note that rebasing this on main, I run into a small dependency issue with |
The core of the old (pre-federation 2.0) composition code has been removed by #1155 but this still left some tangential code that isn't used anymore and should be remove.
This commit remove the whole
federation-js/src/compositiondirectory, which "make sense" since the composition code has otherwise moved to a dedicated module (composition-jsdirectory).I will note that there was some method within the removed files that were exported in practice by
federation-js/src/index.tsthat this remove and could theoretically have been used by federation users, but I didn't spot any obvious candidate (most of what was exported was more related to the supergraph handling). That said, could be worth a double-check during review.