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

Removed logic breaking returning of JSON content #21

Closed
wants to merge 1 commit into from

Conversation

naugtur
Copy link

@naugtur naugtur commented Oct 3, 2014

There is no documentation for a send function in node's HTTP, so I assume it's supposed to be Express response.
Anyway, I have never seen a call to .send with a response object passed as argument in any documentation. On the other hand it breaks passing objects (intended as json responses) to the .send method if they have a field called body.

There is no documentation for a send function in node's HTTP, so I assume its supposed to be Express response. 
Anyway, I have never seen a call to `.send` with a response object passed in any documentation. On the other hand it breaks passing objects (intended as json responses) to the `.send` method if they have a field called body.
@alanjames1987
Copy link
Contributor

I'll take a look at your pull request as soon as I can. I just wanted to confirm I received it.

@howardabrams
Copy link
Collaborator

Hey Alan, where did we leave off with this pr?

@alanjames1987
Copy link
Contributor

I didn't get a chance to look over the pull request. I won't have time to until this weekend.

@estilles
Copy link

estilles commented Mar 7, 2015

@howardabrams / @alanjames1987, @naugtur is correct. If the argument passed to res.send() is an object, Express does not check the object for status codes or use only the body key when present. This is definitely a deviation from Express' implementation.

When Express' res.send() encounters an object it actually passes it directly to res.json() (which in turn stringify's it and passes it to res.send()).

@naugtur's PR set's _data to the passed object, instead of passing it to .json(), which is also inconsistent with Express' implementation.

While we definitely need to address @naugtur's concerns, this PR should not be merged as-is.

@estilles
Copy link

estilles commented Apr 2, 2015

This issue should be take care of once we roll out release 2.0 (see our Roadmap to Release 2.0, issue #54).

@eugef eugef closed this Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants