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

restClient should format http response data consistently for GET_* request types #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benglass
Copy link

@benglass benglass commented Jan 7, 2018

I believe there is a bug in the way convertHTTPResponseToREST is implemented as it does not attempt to format that from GET_ONE requests in the same way that data from GET_LIST requests are formatted.

To reproduce the issue, go directly to an edit page for a resource without coming from the list page (so the item wont be in the store already from the list view), which will trigger it to be loaded via GET_ONE.

h2. What convertHTTPResponseToREST does now

  1. GET_LIST, GET_MANY_REFERENCE: Transforms response.data array elements from JSON API format of { type: 'books', id: 4, { attributes: { name: 'foo' } } to { id: 4, name: 'foo' }. It also translates relationships from the JSON API format of { author: { data: { type: 'people', id: 4 } } } to { author: 4 }
  2. GET_MANY: Transforms response.data array elements in the same way as GET_LIST, GET_MANY_REFERENCE handling but does not touch relationships
  3. GET_ONE: Does no transformation on response.data

I believe this behavior causes a bug where data loaded by GET_ONE is a different shape in the store than data loaded by GET_LIST/GET_MANY. See screenshot below showing store data in different shapes.

Screenshot

I think that response.data should probably always be transformed in the same way, from a JSON-API representation that includes attributes, relationships, etc. to a plain old javascript object. This PR starts in that direction, but can you weigh in on whether my understanding here seems correct and this direction makes sense?

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.

1 participant