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

Remove payload key from response and stick to original format. #123

Merged

Conversation

HorizonXP
Copy link
Contributor

Fixes #122

@syrusakbary
Copy link
Member

syrusakbary commented Mar 3, 2017

Hi @HorizonXP, thanks for the PR!

The batching support was initially coded for supporting react-relay-network-layer (as in https://github.com/nodkz/react-relay-network-layer/blob/4e5edc9096a8f64adff9722adea4f41f8f8f198a/test/batch.test.js#L42-L46 ) (and maybe an earlier version of apollo-client? )

I thought apollo-client was using the same strategy for batching.

@helfer @nodkz it seems the batching query strategy in apollo-client and react-relay-network-layer differs, could be possible to standardize one or other?

Apollo Client

[
  {
    "id": 1,
    "errors": [
      { "location": 1, "message": "major error" },
    ],
  },
  { "id": 2, "data": {} },
]

react-relay-network-layer

{
  "status": 200,
  "body": [
    {
      "id": 1,
      "payload": {
        "errors": [
          { "location": 1, "message": "major error" },
        ],
      },
    },
    { "id": 2, "payload": { "data": {} } },
  ],
}

@helfer
Copy link

helfer commented Mar 3, 2017

@syrusakbary It's actually even simpler for Apollo as it doesn't use an id field. From our perspective there's no point in adding id if all responses come together anyway. If you don't want to send them together, it's a different transport anyway.

I also think the nodkz batching is a bit different from what you described. Status is present on every response: [{id, status, payload }, {id, status, payload}, ... ].

To harmonize the two formats, I would propose dropping the payload field and simply expanding its contents into the response directly. Apollo Client can safely ignore the id and status fields, and for nodkz batching it's just a very simple change. So the format I'm proposing for graphene-django would look like this:

[{
    id,
    status,
    data,
    errors
  },
  // etc ...
] 

What do you think?

PS: @HorizonXP thanks for making the PR to prompt this discussion!

@helfer
Copy link

helfer commented Mar 3, 2017

@HorizonXP make sure you also change the tests, and not just the code. Currently the tests are failing because they still expect payload.

@nodkz
Copy link

nodkz commented Mar 3, 2017

@syrusakbary @helfer i'll look tomorrow what can i do. I'd like idea of harmonization of two formats.

@HorizonXP
Copy link
Contributor Author

I'll update the PR so that it passes the tests, but I can't test against react-relay-network-layer. @nodkz, I trust that you could do this yourself? It looks like it's an external change that shouldn't affect graphene-django.

@coveralls
Copy link

coveralls commented Mar 3, 2017

Coverage Status

Coverage increased (+1.1%) to 93.75% when pulling f720912 on HorizonXP:fix-batch-response-format into 4cc4673 on graphql-python:master.

@nodkz
Copy link

nodkz commented Mar 4, 2017

Just published [email protected] now supports its batch format and apollo batch format. So @syrusakbary you may safely migrate.

@syrusakbary
Copy link
Member

syrusakbary commented Mar 5, 2017

Thanks for the great work! @nodkz @HorizonXP

Merging :)

@syrusakbary syrusakbary merged commit 1139507 into graphql-python:master Mar 5, 2017
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