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

Add callout that external subschema approach is not prohibited. #19

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Feb 2, 2024

I'm overall not convinced that the way the specification is designed should match how it is used. Most teams/groups using composition may be "in control" of all of the different subchemas/source schemas, but I'm not convinced that this definitely so; perhaps they are "in control" of most of them but want to link to popular 3rd party APIs, Github/Stripe, etc. Since the problem of connecting arbitrary subschemas is a superset of what the specification currently proposes, I think it would make sense to start there, as otherwise the specialized case crowds out the more general.

So I would be in favor of a more generic specification that describes how multiple GraphQL schemas can be composed, and then leave room for additional tooling that might be helpful for those teams that are truly "in control" of all of their subschemas, and am leaning against the vision suggested here.

On the other hand, this PR introduces a small change that just highlights that it should be possible to transform arbitrary schemas into compliant source schemas, which I think might bridge the overall gap.

@smyrick
Copy link

smyrick commented Feb 5, 2024

On the other hand, this PR introduces a small change that just highlights that it should be possible to transform arbitrary schemas into compliant source schemas, which I think might bridge the overall gap.

I think this is the gap that we should call out in the spec (maybe in other sections), but that we don't need to solve and leave that to other on how you do schema transforms.

Lets say I do want to use the Stripe GraphQL API. I can download the schema to be used in my composed graph, but we don't want to force companies to add support for this composition spec in their public schema. There is also the fact that you can't really get directive metadata today.

Our spec should make sure that you can make a schema that does not have any composer support compatible with explicit directives (like @finder) and adding those directives to the schema can be done with standard GraphQL extensions or some external transformer library.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Feb 6, 2024

@smyrick I'm not sure if we are disagreeing here; I read your comment that you are agreeing with the small edit I am making in this PR. To run with your example, the spec currently is sort of saying that integrating the Stripe GraphQL API is out of scope; with this edit, it is saying that it is in scope inasmuch as a team can make itself responsible for maintaining the external API as a compliant source schema, by adding directives themselves through a transformation and owning all of the implications of that.

Copy link

@smyrick smyrick left a comment

Choose a reason for hiding this comment

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

You are correct. I apologize as I am new to the WG and how to separate discussions from spec change proposals. The edits you made here look good to me. I am calling out where we might want to consider adding more language elsewhere, but as the WG works through the different sections we can get specifics.

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Feb 8, 2024

No apology necessary!!!! Your comments have been super helpful, just want to make sure we are on the same page. :)

benjie
benjie previously requested changes Feb 8, 2024
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Here's a suggested alternative way of stating this, I think the even as may be hard to parse for some readers and the may would be considered a usage of RFC2119 currently so I wanted to make this a Note to make it non-normative.

spec/Section 1 -- Overview.md Outdated Show resolved Hide resolved
@michaelstaib michaelstaib changed the title add callout that external subschema approach is not prohibited Add callout that external subschema approach is not prohibited. Mar 28, 2024
Comment on lines 21 to 22
schemas.
Note: Tooling may be built to transform existing or external schemas into
Copy link
Member

Choose a reason for hiding this comment

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

spec-md does not support double-space forced newlines because Lee doesn't like that that's a feature of Markdown.

Also, I'm not sure this will work correctly as a "Note" callout under spec-md in this position? I'd have to render it with spec-md to be sure, but I think it would be better somewhere after the bulleted list?

Copy link
Contributor

Choose a reason for hiding this comment

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

It renders like this:

image

If you add an empty line before the note, you get:

image

Better, but ideally it would be indented.

@michaelstaib
Copy link
Member

I think this note is necessary. Tools can always add preprocessing to the process.

@yaacovCR yaacovCR requested a review from benjie December 1, 2024 15:24
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.

5 participants