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

Adapter mixin causes error when API returns empty object & 204 #107

Closed
simonihmig opened this issue Mar 22, 2018 · 5 comments
Closed

Adapter mixin causes error when API returns empty object & 204 #107

simonihmig opened this issue Mar 22, 2018 · 5 comments
Assignees

Comments

@simonihmig
Copy link

Getting this ember-data error after migrating to fetch and using the adapter mixin: Assertion Failed: normalizeResponse must return a valid JSON API document: One or more of the following keys must be present: "data", "errors", "meta".

This happens when the API returns a 204 response, but with an empty objects as the payload ("{}"). Our real API actually doesn't do this, but Mirage's default handler for DELETE calls seems to return this.

It seems this was caused by #60, which changed the way a 204 response is handled in determineBodyPromise().

jQuery's ajax seems to handle this differently. The raw xhr object has a status of 204 and "{}" as the responseText, but the success hook nevertheless receives undefined as the payload, as can be seen on this screenshot:

image

To be a drop-in replacement, I think this should behave the same way as before, i.e. return undefined (or maybe { data: null } as it was before)!?

@nlfurniss nlfurniss self-assigned this Apr 7, 2018
@nlfurniss
Copy link
Collaborator

hey @simonihmig!

Seems like something that should be changed in Mirage. According to httpstatus, A 204 response is terminated by the first empty line after the header fields because it cannot contain a message body.

The logic in ember-fetch assumes that a 204 is being implemented this way

@simonihmig
Copy link
Author

@nlfurniss hey, yes, I should probably create an issue for mirage as well. But I nevertheless think this could maybe be handled a bit better here as well, i.e. ignore any payload in the adapter mixin for a 204 response. Don't you think?

@tchak
Copy link
Collaborator

tchak commented Apr 7, 2018

I just want to mention that we are moving fetch support to Ember Data itself : emberjs/data#5386

@xg-wang
Copy link
Member

xg-wang commented Oct 29, 2018

Hi @simonihmig

To be a drop-in replacement, I think this should behave the same way as before, i.e. return undefined (or maybe { data: null } as it was before)!?

But the PR (it's same as now) you linked, the handling is to return { data: null }

if (response.ok && (status === 204 || status === 205 || requestData.method === 'HEAD')) {
    payload = { data: null };
}

Is it the expected output? Or the payload should be an empty string in 204 case?

@simonihmig
Copy link
Author

Closing this, as this has been fixed in Mirage, and the fetch adapter mixin is deprecated anyway.

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

4 participants