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

Batching: make query id optional #55

Closed
janmeier opened this issue Nov 24, 2017 · 1 comment
Closed

Batching: make query id optional #55

janmeier opened this issue Nov 24, 2017 · 1 comment

Comments

@janmeier
Copy link
Contributor

I'm trying to make the batching network layer work together with apollo-server-express. I'm using apollo-server instead of graphql-express + the middleware from this module, because it supports query and field tracing.

The batch middleware depends on an id for each batched query to connect requests and responses. However, apollo-server does not pass this id back. This actually turns out to be in line with the graphql spec:

To ensure future changes to the protocol do not break existing servers and clients, the top level response map must not contain any entries other than the three described above. [data, errors, and extensions]
http://facebook.github.io/graphql/draft/#sec-Response-Format

However, the spec also defines that JSON serialization is ordered:

Since the result of evaluating a selection set is ordered, the JSON object serialized should preserve this order by writing the object properties in the same order as those fields were requested as defined by query execution.
http://facebook.github.io/graphql/draft/#sec-JSON-Serialization

So it should be possible to correlate responses to requests without relying on ids. A solution could be to make the ID optional, and fall back to query index when there is no id.

I can implement this change if you think it sounds good, but I would like to get some feedback first.

@janmeier janmeier changed the title Batching: make request id optional Batching: make query id optional Nov 24, 2017
@nodkz
Copy link
Collaborator

nodkz commented Nov 24, 2017

Will be great! 👍

Do it as a fallback if sub-responses do not contain id

  • check that req/res has the same length (otherwise throw an error).
  • and switch to ordered correlation.

PS. Try to not break old behavior.
Thanks.

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

No branches or pull requests

2 participants