🦋 Changeset detectedLatest commit: a643098 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Sorry, something went wrong.
There was a problem hiding this comment.
This pull request introduces comprehensive support for BSON types (including BsonBinaryData, BsonObjectId, BsonTimestamp, Decimal128Value, Int32Value, MaxKey, MinKey, and RegexValue) across both the standard and Lite Firestore SDKs. The changes span API definitions, serialization, indexing, query validation, and local store comparisons, supported by new utility classes for 128-bit decimal operations and extensive integration tests. Feedback on the implementation highlights opportunities to improve safety and compatibility, such as using optional chaining to prevent runtime errors when writing BSON timestamp index maps, validating integer inputs for BSON types, using SDK-consistent FirestoreError classes, and replacing ES2022 Uint8Array.prototype.at() with standard index access for broader environment compatibility.
Sorry, something went wrong.
| const fields = mapValue.fields || {}; | ||
| if (fields) { | ||
| // The JS SDK encodes BSON timestamps differently than the backend. | ||
| // This is due to the limitation of `number` in JS which handles up to 53-bit precision. | ||
| this.writeIndexMap( | ||
| fields[RESERVED_BSON_TIMESTAMP_KEY].mapValue!, | ||
| encoder | ||
| ); | ||
| } |
There was a problem hiding this comment.
The if (fields) check is redundant because fields is initialized to mapValue.fields || {} which is always truthy. More importantly, accessing fields[RESERVED_BSON_TIMESTAMP_KEY].mapValue! directly can cause a runtime TypeError if the key is missing. Using optional chaining and checking if the map exists before writing is much safer.
Sorry, something went wrong.
| constructor(readonly subtype: number, data: Uint8Array) { | ||
| if (subtype < 0 || subtype > 255) { | ||
| throw new FirestoreError( | ||
| Code.INVALID_ARGUMENT, | ||
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | ||
| ); | ||
| } | ||
| this.subtype = subtype; |
There was a problem hiding this comment.
The assignment this.subtype = subtype; is redundant because readonly subtype: number in the constructor parameter list automatically declares and initializes it as a class property. Additionally, we should validate that subtype is an integer to prevent floating-point values or NaN from being silently accepted.
| constructor(readonly subtype: number, data: Uint8Array) { | |
| if (subtype < 0 || subtype > 255) { | |
| throw new FirestoreError( | |
| Code.INVALID_ARGUMENT, | |
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | |
| ); | |
| } | |
| this.subtype = subtype; | |
| constructor(readonly subtype: number, data: Uint8Array) { | |
| if (subtype < 0 || subtype > 255 || !Number.isInteger(subtype)) { | |
| throw new FirestoreError( | |
| Code.INVALID_ARGUMENT, | |
| 'The subtype for BsonBinaryData must be a value in the inclusive [0, 255] range.' | |
| ); | |
| } |
Sorry, something went wrong.
| constructor(readonly seconds: number, readonly increment: number) { | ||
| // Make sure 'seconds' and 'increment' are in the range of a 32-bit unsigned integer. | ||
| if (seconds < 0 || seconds > 4294967295) { | ||
| throw new Error( | ||
| "BsonTimestamp 'seconds' must be in the range of a 32-bit unsigned integer." | ||
| ); | ||
| } | ||
| if (increment < 0 || increment > 4294967295) { | ||
| throw new Error( | ||
| "BsonTimestamp 'increment' must be in the range of a 32-bit unsigned integer." | ||
| ); | ||
| } |
There was a problem hiding this comment.
We should validate that seconds and increment are integers to prevent floating-point values or NaN from being silently accepted. Also, using FirestoreError with Code.INVALID_ARGUMENT is much more consistent with the rest of the Firestore SDK API than a generic Error. Note: You will need to import Code and FirestoreError from ../util/error at the top of the file.
Sorry, something went wrong.
| if (bytes.length === 0) { | ||
| throw new Error('Received empty bytesValue for BsonBinaryData'); | ||
| } | ||
| const subtype = bytes.at(0); |
There was a problem hiding this comment.
Uint8Array.prototype.at() is an ES2022 feature and may not be supported in all target environments (older browsers or Node.js versions) without a polyfill. Using standard index access bytes[0] is fully compatible, safer, and more performant.
| const subtype = bytes.at(0); | |
| const subtype = bytes[0]; |
Sorry, something went wrong.
Draft