isBuffer: Check object equality not class name #418
Conversation
If minified, the name of the buffer class may change, breaking the `isBuffer()` function, and thus the library. Since 5167be2 binary fields have been broken for users minifying their source. The `.buffer` attribute would contain the raw BSON blob, rather than just the field's value.
4.2.1 breaks minification, see mongodb/js-bson#418
|
Hello @stefanor thank you for your submission! Unfortunately this causes another issue on nodejs, essentially: global.Buffer != require('buffer').BufferSo buffers constructed using the globally available constructor, which is common practice in Node, won't return true for this isBuffer helper. We will further investigate a solution that is cross platform. |
|
Fair enough. |
Alexander-L-G
added
the
tracked-in-jira
Co-authored-by: Anna Henningsen <[email protected]>
nbbeeken
requested review from
a team,
nbbeeken and
dariakp
and removed request for
a team
|
Thanks! |
| @@ -58,7 +58,8 @@ export function haveBuffer(): boolean { | |||
|
|
|||
| /** Callable in any environment to check if value is a Buffer */ | |||
| export function isBuffer(value: unknown): value is Buffer { | |||
| return typeof value === 'object' && value?.constructor?.name === 'Buffer'; | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| return isUint8Array(value) && typeof (value as any)?.toJSON === 'function'; | |||
| } | |||
There was a problem hiding this comment.
I think it would be better to check for the properties on the buffer that the driver actually needs to exist in order to successfully do its job instead of relying on an arbitrary condition to infer that the object is of the correct type; furthermore, we should add some unit tests around types that are expected to pass the check and the ones that are expected to fail
There was a problem hiding this comment.
I think it would be better to check for the properties on the buffer that the driver actually needs to exist in order to successfully do its job instead of relying on an arbitrary condition to infer that the object is of the correct type;
Soooo... if clean code is the goal here, the way to do that is to just never check whether something is a Buffer, because there's never a good reason to do that (e.g. all Node.js built-in APIs which accept a Buffer also accept all other kinds of Uint8Arrays). Luckily, that's actually pretty easy to do (bonus bugfix included 🙂): #432
There was a problem hiding this comment.
Yeah that was pretty much my thought, will take a look at the new code soon, thanks!
Also fixes NODE-3223 (ensureBuffer ignores byteLength/byteOffset). This is the "proper" alternative to mongodb#418 and matches what e.g. Node.js APIs do.
Also fixes NODE-3223 (ensureBuffer ignores byteLength/byteOffset). This is the "proper" alternative to mongodb#418 and matches what e.g. Node.js APIs do.
Also fixes NODE-3223 (ensureBuffer ignores byteLength/byteOffset). This is the "proper" alternative to mongodb#418 and matches what e.g. Node.js APIs do.
|
Closing this PR in favor of the more general approach in #432. |
isBuffer: Check object equality not class name
If minified, the name of the buffer class may change, breaking the
isBuffer()function, and thus the library.Since 5167be2 binary fields have been broken for users minifying their source. The
.bufferattribute would contain the raw BSON blob, rather than just the field's value.NODE-2963