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

Umbrella Issue: Client side support for Persisted Queries #1080

Closed
5 tasks
rricard opened this issue Dec 22, 2016 · 11 comments
Closed
5 tasks

Umbrella Issue: Client side support for Persisted Queries #1080

rricard opened this issue Dec 22, 2016 · 11 comments

Comments

@rricard
Copy link
Contributor

rricard commented Dec 22, 2016

Effort to implement #478.

Feature's API design

A first design has been described by @Poincare in #478. While I think it could work, I first want to see two things: first, see which design decision we end up with in apollographql/apollo-server#253 and secondly, I want to discuss that in the following thread, I think, the client's behavior could be more simple than that, more static...

  • Reach an API design decision for the client

Understand persisted query docs

  • Be able to get in some way (defined earlier in the API design, probably with an hash) the persisted query id of a given document
  • Send a query with persisted id through the network interface if persisted queries are enabled in the interface

Persist query docs

  • If the previous persisted query query fails, send the original document to /graphql and asynchronously try to persist it if the /persist endpoint is configured.
  • A preprocessor on the codebase should be able to send documents for persistance independently from the client (I'm thinking of https://github.com/apollostack/graphql-document-collector)
@rricard
Copy link
Contributor Author

rricard commented Dec 22, 2016

So to challenge @Poincare's design, I think that some hashing method would be enough to generate a persisted document id (i.e. no need for lookup tables), either we persist it if the server fails to match the id (automatic way of handling persisted queries) or we can preprocess the documents that will "prepare" the server to receive persisted ids. If we go that way though, we have to absolutely ensure that the hashing method stays consistent between platforms!

@stubailo
Copy link
Contributor

Yeah actually that was mostly my decision and I decided to go that way because ensuring the same hashing algorithm in all consumers seemed pretty hard. Plus, if you are already running a build tool to generate types or similar it seemed like not a big step to generate a map of queries to IDs.

One thing I don't understand is why persistence needs to be able to happen at runtime at all. Wouldn't the server just accept any query in dev mode, and some static list of IDs in production?

@stubailo
Copy link
Contributor

(Btw I am open to other ideas as well!)

@rricard
Copy link
Contributor Author

rricard commented Dec 22, 2016

@stubailo I think this is a pretty interesting problem actually (the hashing one). But I thought of something maybe much simpler that would still make maps useless! Let's say we add a directive for Apollo: @apolloPersisted(id: ID!), this directive could be added during pre-compilation. Apollo-client would just read it and send the persisted query id to the server but still use the rest of the query for client-side caching purposes. The advantage is: we don't need to compute hashes at runtime, which is something we both want!

I do agree with you that runtime persistence is not really useful. That is if you take the time to setup the correct tooling. I think that, like with batching, an easy way to setup persistence (with just a flag in the network interface) could be awesome. In this case, the client would handle persisting by itself if it sees missed id matches. However, this will obviously make the client heavier for a case we won't clearly recommend...

@helfer
Copy link
Contributor

helfer commented Dec 23, 2016

Wait, just so I understand: it's not having the same hashing althorithm that's challenging, it's having the query be stringified in the same way that's challenging, right? Hashing algorithms have to be consistent across platforms by definition, and I think it's essential that GraphQL queries are also serialized consistently across different implementations. That said, it doesn't really affect our implementation here, because we can just as well compute the hashes at build-time and then store them together with the query.

I also don't think we should do persistence at run time (although it might be interesting during development).

@rricard
Copy link
Contributor Author

rricard commented Dec 23, 2016

@helfer right, the hashing algorithm is actually made to be consistent 😉 but string handling in js may vary from one engine to an another (although not likely with modern engines). The solution with the directive does not care about it though...

Let's not do persistence at runtime on the first version. Maybe later but again, still not sure it's a great idea...

@skevy
Copy link

skevy commented Jan 7, 2017

Wait, just so I understand: it's not having the same hashing althorithm that's challenging, it's having the query be stringified in the same way that's challenging, right?

Maybe I'm missing something, but couldn't consistent formatting be applied to the query (e.g. whitespace removed, etc), and then hash that? That way there would be consistency across all engines.

@Poincare
Copy link
Contributor

This has been implemented through extractgql (which, by the way, uses graphql's print in order to normalize the query string representation - could also layer a hash function on top of this) so there probably isn't a need for this issue to exist on this repo.

Can we close?

@stubailo
Copy link
Contributor

Yeah there's a good question here about where an issue should live if it's about this project but wouldn't really appear in the particular codebase.

@Poincare
Copy link
Contributor

That's true. I think we should maintain a list of ideas that we'd like implemented around Apollo Client (with the idea tag?) on this repo since that seems like a good place for contributors to reach them.

In this particular case, since there's active development on this thing already, is it safe to close this specific issue?

@helfer
Copy link
Contributor

helfer commented Feb 4, 2017

I agree with @Poincare so I'll close the issue. The docs could use some love though...

@helfer helfer closed this as completed Feb 4, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants