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

Emit send and end events for json response too #24

Closed
wants to merge 1 commit into from

Conversation

andriasang
Copy link

No description provided.

@marneborn
Copy link

I found this same issue today, and am looking at fixing it.

My initial idea was to emit 'json' and then call mockResponse.end(), which emits 'end' and sets _endCalled. However, mockResponse.send() simply emits 'send' and 'end'. And I'd rather be consistent.

Would you rather I change .send to call .end, (and .json to call .send) or make .json act like .send?
The only difference is that _endCalled will be set to true, while right now it's not.

Within express .json (and .jsonp) call .send which then calls .end.
All of the Buffer stuff is done in .send, so the .json method doesn't even really need to be mocked...

@estilles
Copy link

@marneborn, I really appreciate your comments.

I've looked into this as well and I agree both .json() and .jsonp() should call .send() which in turn calls .end().

I do, however, disagree that .json()doesn't need to be mocked. In addition to called .send() it also sets the Content-Type header to application/json (if it isn't already set), among other things. So it definitely need to be mocked as well.

The problem ATT is that simply changing .send() to call .end() introduces a breaking change. (See issue #49). I have scheduled fixing this issue in our 2.0 release, the date of which I will be announcing soon.

@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).

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.

5 participants