search

AmpersandJS / ampersand-sync

Provides sync behavior for updating data from ampersand models and collections to the server.
http://ampersandjs.com
MIT License
20 stars 28 forks source link

Fix failure on 204 in the node environment #79

Closed aclimatt closed 7 years ago

aclimatt commented 7 years ago

Hi,

The problem we were experiencing is that we have an endpoint that returns a 204. With request in the browser, the body is null, however in Node (server-side rendering) the body is an empty string. This causes the JSON parse to fail and the promise to reject only on server-side rendering.

Now, this could potentially be solved by submitting a PR to request to fix this behavior, but for such a big library it's hard to say if this behavior is intended or not, and that change could have sweeping consequences. So it's probably unlikely that we could change that behavior.

wraithgar commented 7 years ago

Hi! Thank you for taking the time to submit the PR for this. Would you be able to add a test for this to test/integration.js? I think it would be fine to have it test specifically the use case you outlined here (204 body w/ empty response).

aclimatt commented 7 years ago

Hi, thanks for your patience! I've been having some trouble writing a test for this. I don't think zuul supports sending empty string bodies in responses, which actually might be a bug in their code, because empty string responses (for example, in text/html) should totally be supported.

I'm trying to test the behavior with this line: requests[0].respond(204, { "Content-Type:": "application/json" }, '');

Which is what Node sends in this case. However, I'm getting an error from zuul:

Error: TypeError: Requested keys of a value that is not an object.

That's happening over here I believe, where it's trying to parse the body: https://github.com/defunctzombie/zuul/blob/master/lib/PhantomBrowser.js#L100

Interestingly enough, this line does work: requests[0].respond(204, { "Content-Type:": "application/json" }, ' '); (a string with one space). However, that does not model the correct behavior, and doesn't pass the test with the fix we implemented.

Any advice? Is there a better way to test this that maybe I'm not seeing? I'm also not familiar with zuul, so perhaps you know more about its functionality that we could use?

Thanks!

wraithgar commented 7 years ago

It seems like you did quite a bit of digging and if you couldn't get zuul to send the right data I'm satisfied with your efforts. Thank you for being so thorough.

I'm gonna give this a +1 and say we can merge this if we get another +1 from another core member.

aclimatt commented 7 years ago

Of course, I'm definitely happy to write a test for this, I just don't quite know how to model the same response that Node sends using zuul without it crashing. Thank you!

dhritzkiv commented 7 years ago

lgtm!

wraithgar commented 7 years ago

Published as v5.0.1

Thank you for taking the time to work on this @aclimatt!