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

fix(federation): Remove the notion of @inaccessible from subgraphs #880

Merged
merged 4 commits into from
Jul 15, 2021

Conversation

trevor-scheer
Copy link
Member

@trevor-scheer trevor-scheer commented Jul 15, 2021

We've decided to go forward with the decision to ignore the usage of @inaccessible at the subgraph level, and instead allow @inaccessible to be a result of using @tag + Studio governance.

This directly relates to #869 but will take precedence due to the updated decision.

This PR also handles the rename of appliedDirectives recommended in feedback on #859.

Fixes #867.

Copy link
Member

@glasser glasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevor-scheer says that making progress today on another PR will be much easier if this PR is merged now, and that appropriate reviewers aren't available in this time zone now. Clicking the green button without a real review; @trevor-scheer will find somebody to do post-merge review.

@pcmanus
Copy link
Contributor

pcmanus commented Jul 16, 2021

Had a quick post-merge review look and didn't spot any obvious issue.

One remark that crossed my mind is: what we mostly wanted here was the updates to compose.ts so it doesn't handle/propagate @inaccessible on subgraphs, but as we kind of want to allow studio to transform @tag into @inacessible in the supergraph, would it have made sense to preserve the handling of @inacessible in printSupergraphSdl.ts? I suppose it's somewhat moot though if studio doesn't use printSupergraphSdl.ts (does it?).

pcmanus pushed a commit that referenced this pull request Jul 16, 2021
@abernix
Copy link
Member

abernix commented Jul 16, 2021

would it have made sense to preserve the handling of @inacessible in printSupergraphSdl.ts?

Good question! I found an internal thread which helped me answer this. Preserving the handling may still make sense but currently that functionality is not necessitated by the Studio team. We can re-introduce it if they need it for their use cases but since that is the only current use-case (that I'm aware of) I think it's fine to leave it out.

cc @jsegaran in case you want to propose it be introduced eventually!, but I can also see this need being superseded by a more principled way of directive life-cycle management that we already have in mind.

@abernix abernix added this to the MM-2021-07 milestone Jul 16, 2021
@trevor-scheer
Copy link
Member Author

Thanks for taking a look and your intuition matched mine @pcmanus. As @abernix mentioned, I did end up considering this (after the fact) and fortunately it seems Studio doesn't need that functionality. If we ever do need to reintroduce it though, this PR will make for a great reference of what to re-undo 😄

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.

composeAndValidate() unexpectedly generates invalid GraphQL when using @tag/@inaccessible
4 participants