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

Persisted Documents: encourage URL approach #305

Draft
wants to merge 1 commit into
base: persisted-documents
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Aug 6, 2024

This is just an early draft; I ran out of time.

Comment on lines +243 to +248
For example, if the _GraphQL endpoint_ is `https://example.com/graphql` then a
persisted document request may be made to an endpoint such as
`https://example.com/graphql/sha256:517c56d2ba0779653b7698881207f749509f331bdaccbe951a82c378bc869556/FriendNames`.
For documents containing a single anonymous operation the final segment must be
omitted, e.g.
`https://example.com/graphql/sha256:71f7dc5758652baac68e4a10c50be732b741c892ade2883a99358f52b555286b`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe namespace persisted documents under /graphq/<prefix>/<sha>?

e.g.

  • /graphql/persisted/<sha>/<operation_name>
  • /graphql/documents/<sha>/<operation_name>
  • /graphql/persisted-documents/<sha>/<operation_name>

Main reason for this is that graphql-sse already encourages /graphql/stream, which in theory can conflict with a "custom" hash. Changing this would also leave the options open to add other URL based features later on and not conflict with the persisted documents.

Copy link
Member

Choose a reason for hiding this comment

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

We do namespace it in HotChocolate for less collision potential. However, Twitter for instance puts it directly on the GraphQL route as as outlined in this document.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, this is one of the things I was considering changing before moving this out of draft. I think I also want to change it to /graphql/persisted/<operation_name>/<hash> so that the op-name comes first for easier debugging. If there's no op-name, we can just use - for that part of the URL, e.g. /graphql/persisted/-/<hash>.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a big fan of having the operation name first. 🤔

From a server perspective, we first look up the hash for the document, and then the operation name within that document. Having the operation name first seems wrong to me.

From a debugging perspective, it does not matter too much whether the operation name comes before the hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also if we are looking at Chrome DevTools, the last URL part is displayed. So replacing operation name and hash might even be counter productive.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, re Chrome devtools. Okay lets stick with the hierarchical approach, it makes more sense anyway 👍

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