Skip to content
This repository has been archived by the owner on Aug 20, 2020. It is now read-only.

Add hash_ids flag to override default integer values #35

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

Conversation

jasonmorita
Copy link

When experimenting with using persisted queries, I found that trying have multiple clients caused potential query ID collisions due to the use of incremental integers as IDs.

This adds the option to use a hash of the query as the ID to allow multiple query maps to be combined and used on the server.

@apollo-cla
Copy link

@jasonmorita: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -376,7 +405,7 @@ describe('addPersistedQueries', () => {
const networkInterface = new GenericNetworkInterface();
addPersistedQueries(networkInterface, queryMap);
const expectedId = queryMap[getQueryDocumentKey(request.query)];
return networkInterface.query(request).then((persistedQuery: persistedQueryType) => {
Copy link
Author

Choose a reason for hiding this comment

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

This was giving a TS error.

@jasonmorita
Copy link
Author

@stubailo Hi, just a bump :)

@Poincare
Copy link
Contributor

Hi @jasonmorita - sorry for the delay. I'll look at this shortly.

@jasonmorita
Copy link
Author

jasonmorita commented Aug 19, 2017

@Poincare After thinking about this some more in the last few weeks, we have started to realize that it might be helpful to also use the operation name in the key. Like { "query getUsersQuery() {...": "getUsersQuery-<hash>" }

What are your thoughts on that?

The use-case we have is that we have more than one client, but we want to use persisted queries and have them reference the same queries on the GQL server by some sort of human readable name.

I think the short-term goal would be to generate the query map on the client based on the queries a React app is using and then store that map on the server.

@Poincare
Copy link
Contributor

@jasonmorita I think the relevant issue here is #34. There will likely be an effort to change the structure that persistgraphql uses to align with the one with apollo-codegen. I haven't had a chance to look into the proposal very deeply yet but once I do, I think we'll have a clear path forward both here and in #34.

@mergebandit
Copy link

Any update on this? <3

@Poincare
Copy link
Contributor

@mergebandit Basically, we have to move to using apollo-codegen's hashing scheme rather than doing something ad-hoc in here so I don't think these particular changes will be merged.

@jasonmorita
Copy link
Author

@Poincare Is this being actively worked on? What's the path forward for PQs?

@jkrems
Copy link

jkrems commented Dec 20, 2017

@Poincare Looks like apollo-codegen uses sha256 instead of sha512. Is that the main concern?

@jasonmorita
Copy link
Author

@austinmcorso Hey, is this gonna be merged?

@austinmcorso
Copy link

@jasonmorita I'm not a maintainer, do require similar functionality so hoping it does

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

Successfully merging this pull request may close these issues.

6 participants