Skip to content

fix the case when toJSON() returns a Buffer #2

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

Closed
wants to merge 2 commits into from

Conversation

chatziko
Copy link

@chatziko chatziko commented Jan 8, 2016

has-binary assumes that toJSON() returns an object, but this is not necessary.
It incorrectly returns false for objects whose toJSON() returns a Buffer, eg:

{ a: 'a', toJSON: function() { return new Buffer('abc') } }

Returning Buffer from toJSON is useful for classes who want to serialize themselves in binary when sent over socket.io.

The fix is simple, I also added a test.

@darrachequesne
Copy link
Member

Merged as #6, thanks!

@chatziko
Copy link
Author

chatziko commented Mar 1, 2017

Thanks for the merge!

Maybe it's a detail, but if the aim of has-binary is to match JSON.stringify, then toJSON() should not be called again in the object returned by toJSON(). In other words:

> var o = { toJSON: function() { return this } }
> JSON.stringify(o)
'{}'
> hasBinary(o)
RangeError: Maximum call stack size exceeded

That's why my fix was a bit more complicated than a recursive call.

@darrachequesne
Copy link
Member

@chatziko #7 what do you think?

@chatziko
Copy link
Author

Looks good.

(looking forward to a release to stop maintaining my own fork :D)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants