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

Don't skip afterFetch in cases where there's no response body. #35

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

Conversation

zpencerq
Copy link
Contributor

@zpencerq zpencerq commented Jul 11, 2019

The original error was that the afterFetch middleware functions weren't being called because of various short-circuiting logic within Request#_handleResponse.

Behavior changes

  • When sending a DELETE and getting a 200 response, it will now try to parse that body. Prior to this change-set, it would skip.
  • An invalid JSON document is now determined by both data and meta being undefined rather than just data (aside from a JSON parse error).
    • This is informed by the previous link as well, since only meta being defined is valid.
  • afterFetch calls happen with json=null in the case of a 204 No Content.
    • Presumably this will affect existing middlewares.

Questions:

  • The spec for deleting resources does not specify if a response body MUST be returned. However, you could interpret that "a deletion request has been accepted for processing ... and no content is returned" requires that a 204 be returned by the server. Should we remove the ability for non-204 responses to be empty?

  • The default json body of null was chosen instead of an empty object because it's representing the absence of a body; I figured just {} would be confusing (but I also don't like giving people NPE). Do you think we should introduce some Null Object default that'll explain where it came from (empty body) on field access?

For some reason, the prior method of constructing a Response
was having a status code of 200.
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