-
Notifications
You must be signed in to change notification settings - Fork 253
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
fix(NODE-3821): nullish check before using toBSON override function #477
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cplepage, thanks so much for contributing here! I think this is something we would certainly want to fix. I've gone ahead and created a JIRA ticket to track the work and I have some suggestions of improvements for ya. If you get the chance adding a test of your usecase to: test/node/to_bson_test.js
would be a great help.
@dariakp @durran @baileympearson, Just wanna get y'all's attention here I'm suggesting we drop the throw case with toBSON not being of type 'function'. While very unlikely it seems wrong that our BSON library would not allow the document: { toBSON: 1 }
to be serialized. That being said we could easily keep it, and revisit that at a later time.
Co-authored-by: Neal Beeken <[email protected]>
Co-authored-by: Neal Beeken <[email protected]>
@nbbeeken good idea on the optional chaining, looks clean! |
Hey @cplepage do you mind if I push up some changes of my own here? due to our support for older node versions the testing arrangement can be kinda strange, I can make sure we still get bigint coverage on later versions of node 🙂 |
@nbbeeken please go ahead! Tried a few things for the tests, but CI never passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cplepage Let me know if you see anything I missed, I'll ask the team for a final look now 🙂 thanks again!
@nbbeeken Looks good! Thanks to you for the quick replies!! |
Description
In the serializer, when it comes to check for an override value (
toBSON
), changing thevalue
check to compare withnull
andundefined
instead of relying on the truthiness ofvalue
.What is the motivation for this change?
Cannot extend BigInt prototype with
.toBSON()
since0n === false
trying to make this work