Skip to content
This repository has been archived by the owner on Feb 6, 2018. It is now read-only.

Return null for no-content responses with fetch #84

Merged
merged 3 commits into from
Nov 8, 2016
Merged

Return null for no-content responses with fetch #84

merged 3 commits into from
Nov 8, 2016

Conversation

bedeoverend
Copy link
Contributor

Checks if the response is 204 when using fetch and return null body. Fixes #83

@bedeoverend
Copy link
Contributor Author

@daffl so this isn't ready to merge yet - what I've added in has broken quite a few tests, though I'm not quite sure why or what's going on - I don't think I'm familiar enough with the tests setup.

I'm also not really satisfied with the test setup i.e. without the 'fix' the tests fail saying Expected {} === null, rather than throwing the expected error as outlined in #83. But I don't think we can get around that until node-fetch/node-fetch#165 is resolved / reverted.

return null;
}

response.json();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think arrow functions with brackets have an implicit return so this will probably have to be return response.json();.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, that's embarrassing. Yup you're exactly right, patched.

@daffl
Copy link
Member

daffl commented Nov 7, 2016

@bedeoverend I made a comment. I think you just need to add a return.

@bedeoverend
Copy link
Contributor Author

Thanks @daffl - can't believe I missed that one. That's done the trick, looks all good now. Tried it out in a local project I'm working on that I discovered the bug initially in, and works well now.

@daffl daffl merged commit b28a80e into feathersjs-ecosystem:master Nov 8, 2016
@daffl
Copy link
Member

daffl commented Nov 8, 2016

Perfect, thank you! Released in v1.5.2. feathers-client will be updated once the tests pass.

@bedeoverend
Copy link
Contributor Author

Sounds good. No worries - thanks for all the hard work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants