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

Fixed issue with record -> replay body encoding #1012

Closed
wants to merge 1 commit into from

Conversation

simlu
Copy link

@simlu simlu commented Oct 30, 2017

This matches the replay decode check in https://github.com/node-nock/nock/blob/master/lib/request_overrider.js#L325

Ideally we would also add a corresponding check for isBinaryRequestBodyBuffer (https://github.com/node-nock/nock/blob/master/lib/request_overrider.js#L347):

if((common.isContentEncoded(headers) || isBinaryRequestBodyBuffer) && common.isBinaryBuffer(mergedBuffer)) { ...

However I wanted to keep this pr small and get some feedback.

This matches the replay decode check in https://github.com/node-nock/nock/blob/master/lib/request_overrider.js#L325

Ideally we would also add a corresponding check for isBinaryRequestBodyBuffer (https://github.com/node-nock/nock/blob/master/lib/request_overrider.js#L347):
> if((common.isContentEncoded(headers) || isBinaryRequestBodyBuffer) && common.isBinaryBuffer(mergedBuffer)) { ...

However I wanted to keep this pr small and get some feedback.
@simlu
Copy link
Author

simlu commented Nov 1, 2017

So I was thinking about this more. The appropriate way of doing this is to store the binary or hex information in the recoding and not try and guess it. Agreed?

@ierceg
Copy link
Contributor

ierceg commented Nov 6, 2017

@simlu Thank you for the PR. We would however need tests to show what was wrong with the previous behavior and how this change fixes it.

Also, I'm not following re storing the binary/hex information in the recording - is there any metadata that we aren't capturing?

@simlu
Copy link
Author

simlu commented Nov 13, 2017

@ierceg All meta information is captured. However the logic to determine if the data should be stored/loaded as binary is different. Centralizing this logic would also solve this issue. However it might be easier to just store that information as binary=true/false.

@stale
Copy link

stale bot commented Sep 13, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

@lock
Copy link

lock bot commented Oct 4, 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 Oct 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants