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

Don't apply @shareable if it's already @shareable #2043

Merged
merged 4 commits into from
Aug 11, 2022

Conversation

benweatherman
Copy link
Contributor

In the course of updating to fed2, it's feasible that subgraphs may want to add @shareable applications without @linking to federation/2.0 and importing it from there.

Previously, this would throw an error because the schema upgrader will try to add a @shareable application that has already been manually added. Now we'll check first.

In the course of updating to fed2, it's feasible that subgraphs may want to add `@shareable` applications without `@link`ing to federation/2.0 and importing it from there.

Previously, this would throw an error because the schema upgrader will try to add a `@shareable` application that has already been manually added. Now we'll check first.
@netlify
Copy link

netlify bot commented Aug 5, 2022

Deploy Preview for apollo-federation-docs ready!

Name Link
🔨 Latest commit fe85840
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/62f541439a4a1c000a7d4e53
😎 Deploy Preview https://deploy-preview-2043--apollo-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 5, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@benweatherman benweatherman added this to the 2.1.0 milestone Aug 10, 2022
@benweatherman benweatherman enabled auto-merge (squash) August 11, 2022 17:50
@benweatherman benweatherman merged commit 3990af4 into main Aug 11, 2022
@benweatherman benweatherman deleted the weatherman/dont-apply-shareable-twice branch August 11, 2022 19:32
@pcmanus
Copy link
Contributor

pcmanus commented Aug 16, 2022

In the course of updating to fed2, it's feasible that subgraphs may want to add @Shareable applications without @linking to federation/2.0 and importing it from there.

I don't mind that change much because any risk seems pretty remote in the first place and maybe the added convenience is worth it, but on the hopeful belief that even late feedback can have some benefits, I'll make 2 remarks:

  1. We're implicitly assuming that if a fed1 schema declares a @shareable directive, then it's our @shareable directive. But in theory, an unlucky user might have a pre-existing unrelated @shareable directive, and this patch may make upgrade somewhat more confusing for that hypothetical user.
  2. I wonder if we should "encourage" user to do the upgrade steps that this patch allows, which is to add @shareable manually to the fed1 schema before switching it to a fed2 schema (I reckon that committing that patch is not exactly equivalent to encouraging the step; I guess this point is is me arguing for not encouraging it even if we technically support it). It feels a bit hacky (you'd probably will want to remove the @shareable definition later for instance) and adding @shareable is imo by far the biggest task to switch a fed1 schema to a fed2 one, so if you do that, it makes some sense imo to go all the way to a fed2 schema. I do get that this can theoretically allow to add @shareable to a given subgraph more incrementally, but I question if publishing a schema where @shareable has only be added halfway is a great idea, as this feel this could get confusing. Another way to put it is that seeing a schema with @shareable was a simple and relatively sure way to know a given subgraph has been fully upgraded, but this patch muddle that and you can have a schema where some field/type are marked @shareable and others are not but they are actually shareable.

Again, not asking for a revert or anything like that, especially since this shipped in 2.1.0-alpha4 already. The change does have pros and is fine, but I think it also has a few cons imo and I didn't saw those discussed so wanted to document them.

@smyrick
Copy link
Member

smyrick commented Aug 16, 2022

this patch muddle that and you can have a schema where some field/type are marked @shareable and others are not but they are actually shareable.

Is it possible to add the fact that certain types were modified to the composition hints or logs? It would be helpful to devs to know more explicitly that this is happening when they are running Fed 1 subgraph with Fed 2 composition and it would give them a checklist to go back and improve on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants