feat(firestore): add public type guards for FieldValue sentinels#8102
feat(firestore): add public type guards for FieldValue sentinels#8102kyungseopk1m wants to merge 2 commits intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several static type guard methods to the FieldValue class—specifically isServerTimestamp, isIncrement, isDecrement, isArrayUnion, and isArrayRemove—to facilitate the identification of sentinel values. The changes include the necessary implementation logic, a private helper for decrement detection, comprehensive unit tests, and updated public type definitions. The feedback suggests adding an isDelete type guard for the delete sentinel to maintain API completeness and consistency across the library.
| static isArrayRemove(value: unknown): value is FieldValue { | ||
| return value instanceof ArrayRemoveTransform; | ||
| } |
There was a problem hiding this comment.
For completeness and API consistency, consider adding a type guard for the delete sentinel as well. Since FieldValue.delete() is a core sentinel value alongside the others introduced here, providing isDelete() would ensure full coverage of the public FieldValue factory methods.
static isArrayRemove(value: unknown): value is FieldValue {
return value instanceof ArrayRemoveTransform;
}
/**
* Returns `true` if the provided value is a sentinel returned by
* {@link FieldValue.delete}.
*
* @param value The value to check.
* @returns `true` if `value` is a delete sentinel.
*
* @example
* ```
* Firestore.FieldValue.isDelete(Firestore.FieldValue.delete()); // true
* ```
*/
static isDelete(value: unknown): value is FieldValue {
return value instanceof DeleteTransform;
}| /** | ||
| * Returns `true` if the provided value is a sentinel returned by | ||
| * {@link FieldValue.arrayRemove}. | ||
| * | ||
| * @param value The value to check. | ||
| * @returns `true` if `value` is an array-remove sentinel. | ||
| */ | ||
| static isArrayRemove(value: unknown): value is FieldValue; |
There was a problem hiding this comment.
If isDelete is added to the implementation, it should also be declared in the public type definitions for consistency.
static isArrayRemove(value: unknown): value is FieldValue;
/**
* Returns `true` if the provided value is a sentinel returned by
* {@link FieldValue.delete}.
*
* @param value The value to check.
* @returns `true` if `value` is a delete sentinel.
*/
static isDelete(value: unknown): value is FieldValue;Following the same pattern as the other four sentinel guards, expose a public `isDelete` static method so users can detect sentinels returned by `FieldValue.delete()` without relying on internal class names. Addresses review feedback on googleapis#8102.
|
Thanks @gemini-code-assist! Added |
|
Thanks for the update, @kyungseopk1m. The addition of |
Summary
Adds five public static type guards on
FieldValueso users can detect sentinel values without relying on internal class names likeNumericIncrementTransform.Addresses firebase/firebase-admin-node#2957 (external contribution invited by @ehsannas).
FieldValue.isServerTimestamp(value)FieldValue.isIncrement(value)FieldValue.isDecrement(value)— strict subset ofisIncrement, true only when the operand is negativeFieldValue.isArrayUnion(value)FieldValue.isArrayRemove(value)Each guard is a TypeScript type predicate returning
value is FieldValue.Design notes
isDecrementis documented as a strict subset ofisIncrement(same underlyingNumericIncrementTransform, narrowed by a negative operand). An alternative "partition" design (isIncrement= positive,isDecrement= negative, zero = neither) was considered; the subset design was chosen soisIncrementconsistently behaves as a type detector for any increment sentinel. Happy to flip this if you prefer the partition semantics.instanceofagainst existing internal transform classes — no new runtime surface, no changes to proto serialization, transforms, or validation paths.Testing
handwritten/firestore/dev/test/field-value.tsfor all five guards: match / non-match / primitive / delete-sentinel / cross-sentinel cases, plus explicitisDecrement⊂isIncrementassertion.mocha build/test/field-value.js→ 41 passing.mocha build/test) → 1084 passing, 47 pending (no regressions).