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

Do not automatically consume responses, respect original consumer. #503

Merged
merged 1 commit into from
Mar 20, 2016

Conversation

satazor
Copy link
Contributor

@satazor satazor commented Mar 20, 2016

Fixes #486

After investigation, the issue is that the recorder assumes that all listeners are attached after http.request's callback is called which might not be the case. That's exactly what was happening with me because since got is using promises, the data event was only attached in the next event loop.

Test case & fix added, I haven't committed the browserify bundle. I've tried to follow the coding style, let me know if something needs to be changed.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.614% when pulling 8795717 on satazor:request-promise into 694c421 on node-nock:master.

@satazor satazor mentioned this pull request Mar 20, 2016
@satazor
Copy link
Contributor Author

satazor commented Mar 20, 2016

Do not merge yet, will improve the test to actually run on node 0.10.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.614% when pulling 4ed346e on satazor:request-promise into 694c421 on node-nock:master.

@satazor
Copy link
Contributor Author

satazor commented Mar 20, 2016

Good to be merged.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.614% when pulling 1e8f0d3 on satazor:request-promise into 694c421 on node-nock:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.614% when pulling 477f159 on satazor:request-promise into 694c421 on node-nock:master.

@pgte pgte merged commit 477f159 into nock:master Mar 20, 2016
@pgte
Copy link
Member

pgte commented Mar 20, 2016

@satazor thanks!

Landed on v7.5.0

@satazor
Copy link
Contributor Author

satazor commented Mar 20, 2016

<3

@lock
Copy link

lock bot commented Sep 14, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 14, 2018
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.

3 participants