Skip to content

feat(firestore): add public type guards for FieldValue sentinels#8102

Open
kyungseopk1m wants to merge 2 commits intogoogleapis:mainfrom
kyungseopk1m:feat/field-value-sentinel-public-api
Open

feat(firestore): add public type guards for FieldValue sentinels#8102
kyungseopk1m wants to merge 2 commits intogoogleapis:mainfrom
kyungseopk1m:feat/field-value-sentinel-public-api

Conversation

@kyungseopk1m
Copy link
Copy Markdown

Summary

Adds five public static type guards on FieldValue so users can detect sentinel values without relying on internal class names like NumericIncrementTransform.

Addresses firebase/firebase-admin-node#2957 (external contribution invited by @ehsannas).

  • FieldValue.isServerTimestamp(value)
  • FieldValue.isIncrement(value)
  • FieldValue.isDecrement(value) — strict subset of isIncrement, true only when the operand is negative
  • FieldValue.isArrayUnion(value)
  • FieldValue.isArrayRemove(value)

Each guard is a TypeScript type predicate returning value is FieldValue.

Design notes

  • Shape matches the "option 1" suggestion in [FR] Public API to Detect FieldValue.increment() firebase/firebase-admin-node#2957.
  • isDecrement is documented as a strict subset of isIncrement (same underlying NumericIncrementTransform, narrowed by a negative operand). An alternative "partition" design (isIncrement = positive, isDecrement = negative, zero = neither) was considered; the subset design was chosen so isIncrement consistently behaves as a type detector for any increment sentinel. Happy to flip this if you prefer the partition semantics.
  • Increment/decrement amount accessors are deliberately out of scope here — the original issue raised the question but did not commit to an answer. Happy to follow up in a separate PR if wanted.
  • Uses instanceof against existing internal transform classes — no new runtime surface, no changes to proto serialization, transforms, or validation paths.

Testing

  • New unit coverage in handwritten/firestore/dev/test/field-value.ts for all five guards: match / non-match / primitive / delete-sentinel / cross-sentinel cases, plus explicit isDecrementisIncrement assertion.
  • mocha build/test/field-value.js → 41 passing.
  • Full unit suite (mocha build/test) → 1084 passing, 47 pending (no regressions).

@kyungseopk1m kyungseopk1m requested a review from a team as a code owner April 23, 2026 07:57
@product-auto-label product-auto-label Bot added the api: firestore Issues related to the Firestore API. label Apr 23, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +338 to +340
static isArrayRemove(value: unknown): value is FieldValue {
return value instanceof ArrayRemoveTransform;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;
  }

Comment on lines +2814 to +2821
/**
* 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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.
@kyungseopk1m
Copy link
Copy Markdown
Author

Thanks @gemini-code-assist! Added isDelete (plus its declaration in firestore.d.ts and unit tests) in ca2cc57. The new commit keeps the same instanceof-based pattern as the other four guards.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Thanks for the update, @kyungseopk1m. The addition of isDelete and the corresponding updates to the type definitions and unit tests look correct and consistent with the existing instanceof pattern used for the other sentinel guards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant