feat(NODE-4793): add Symbol.toStringTag to all BSON types#880
feat(NODE-4793): add Symbol.toStringTag to all BSON types#880sarthaksoni25 wants to merge 1 commit intomongodb:mainfrom
Conversation
72ffb34 to
c66fe9c
Compare
| it(`UUID inherits Symbol.toStringTag from Binary`, () => | ||
| expect(Object.getPrototypeOf(TypeClass.prototype)).to.have.property( | ||
| Symbol.toStringTag, | ||
| 'Binary' | ||
| )); |
There was a problem hiding this comment.
Approach is right - matches the existing bsonType getter pattern on BSONValue. One suggestion: override Symbol.toStringTag on UUID to return 'UUID'. _bsontype is a wire-protocol key and must stay 'Binary' (serializer dispatches on it), but Symbol.toStringTag is a developer-facing identity hint with no wire impact - making it reflect the JS class gives nicer debug output ([object UUID]) without breaking anything. The test would then assert toStringTag === TypeClass.name uniformly across all types.
There was a problem hiding this comment.
> Object.prototype.toString.call(Buffer.alloc(0))
'[object Uint8Array]'It is not always the case that the string tag matches the class name, because UUID is a trivial subclass of Binary and Binary is the relevant BSON type it may make sense to keep these matching the _bsontype field. It also is not only a developer hint here's an example where it's a load bearing type check.
An additional benefit to adding this field is it adds the same functionality we added when we introduced the @@mdb.bson.type symbol but without the need to import/create something from the library.
Just my opinion here :) not strongly held
tadjik1
left a comment
There was a problem hiding this comment.
hi @sarthaksoni25, thanks for you work, everything looks correct, simple and clean implementation, well done!
I left one suggestion, please let me know what you think.
|
Should this be considered semver-major? |
|
hi @sarthaksoni25, after discussing this change with the team we have decided against it. the main reason is that it can break existing functionality of external packages that might rely on the string representation of BSON instances. and potential benefits do not justify complexity of the migration. |
Description
Summary of Changes
Adds
Symbol.toStringTagto all BSON types by implementing the getter in theBSONValuebase class, delegating tothis._bsontype. Since_bsontypeis abstract onBSONValueand implemented by every subclass, all 13 BSON types inherit the correct tag automatically.Before:
Object.prototype.toString.call(new ObjectId()) // '[object Object]'
After:
Object.prototype.toString.call(new ObjectId()) // '[object ObjectId]'
Notes for Reviewers
Follows the same pattern as the existing
bsonTypesymbol getter added in NODE-7255. The implementation is a single getter onBSONValue— no changes needed in individual subclasses.Double check the following
npm run check:lint)feat(NODE-4793): add Symbol.toStringTag to all BSON typesbson_type_classes.test.ts)