Skip to content

fix: remove NO_ID_FIELD sentinel from data hooks when idField is not provided #738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tyler-reitz wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −10
Open

fix: remove NO_ID_FIELD sentinel from data hooks when idField is not provided #738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/database.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ export function useDatabaseObject<T = unknown>(ref: DatabaseReference, options?:
}

export function useDatabaseObjectData<T>(ref: DatabaseReference, options?: ReactFireOptions<T>): ObservableStatus<T> {
const idField = options ? checkIdField(options) : 'NO_ID_FIELD';
const observableId = `database:objectVal:${ref.toString()}:idField=${idField}`;
const idField = options ? checkIdField(options) : undefined;

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

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

This ternary expression is redundant. The checkIdField function already handles cases where options is undefined by returning undefined. You can simplify this to a direct call to checkIdField(options).

Suggested change
const idField = options ? checkIdField(options) : undefined;
const idField = checkIdField(options);

@tyler-reitz tyler-reitz Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Declining: the ternary guard at the call site is intentional. checkIdField and checkOptions are typed to expect a real ReactFireOptions object, and the hooks handle the no-options case before calling them. Widening the signatures to accept undefined would require options: ReactFireOptions | undefined since field follows as a required parameter, which isn't a meaningful improvement. The current shape keeps the function signatures strict.

const observableId = `database:objectVal:${ref.toString()}:idField=${idField ?? 'none'}`;
const observable$ = objectVal<T>(ref, { keyField: idField });

return useObservable(observableId, observable$, options);
Expand All @@ -59,8 +59,8 @@ export function useDatabaseListData<T = { [key: string]: unknown }>(
ref: DatabaseReference | DatabaseQuery,
options?: ReactFireOptions<T[]>
): ObservableStatus<T[] | null> {
const idField = options ? checkIdField(options) : 'NO_ID_FIELD';
const observableId = `database:listVal:${getUniqueIdForDatabaseQuery(ref)}:idField=${idField}`;
const idField = options ? checkIdField(options) : undefined;

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

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

This ternary expression is redundant. The checkIdField function already handles cases where options is undefined by returning undefined. You can simplify this to a direct call to checkIdField(options).

Suggested change
const idField = options ? checkIdField(options) : undefined;
const idField = checkIdField(options);

@tyler-reitz tyler-reitz Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Declining: the ternary guard at the call site is intentional. checkIdField and checkOptions are typed to expect a real ReactFireOptions object, and the hooks handle the no-options case before calling them. Widening the signatures to accept undefined would require options: ReactFireOptions | undefined since field follows as a required parameter, which isn't a meaningful improvement. The current shape keeps the function signatures strict.

const observableId = `database:listVal:${getUniqueIdForDatabaseQuery(ref)}:idField=${idField ?? 'none'}`;
const observable$ = listVal<T>(ref, { keyField: idField });
return useObservable(observableId, observable$, options);
}
12 changes: 6 additions & 6 deletions src/firestore.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ export function useFirestoreDocOnce<T = DocumentData>(ref: DocumentReference<T>,
* Subscribe to Firestore Document changes and unwrap the document into a plain object
*/
export function useFirestoreDocData<T = unknown>(ref: DocumentReference<T>, options?: ReactFireOptions<T>): ObservableStatus<T | undefined> {
const idField = options ? checkIdField(options) : 'NO_ID_FIELD';
const idField = options ? checkIdField(options) : undefined;

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

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

This ternary expression is redundant. The checkIdField function already handles cases where options is undefined by returning undefined. You can simplify this to a direct call to checkIdField(options).

Suggested change
const idField = options ? checkIdField(options) : undefined;
const idField = checkIdField(options);

@tyler-reitz tyler-reitz Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Declining: the ternary guard at the call site is intentional. checkIdField and checkOptions are typed to expect a real ReactFireOptions object, and the hooks handle the no-options case before calling them. Widening the signatures to accept undefined would require options: ReactFireOptions | undefined since field follows as a required parameter, which isn't a meaningful improvement. The current shape keeps the function signatures strict.


const observableId = `firestore:docData:${ref.firestore.app.name}:${ref.path}:idField=${idField}`;
const observableId = `firestore:docData:${ref.firestore.app.name}:${ref.path}:idField=${idField ?? 'none'}`;

@armando-navarro armando-navarro Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small edge in the new key format (applies to all five changed key templates): 'none' has the same ambiguity the description calls out for undefined.

  • A component passing { idField: 'none' } lands on the same cache key as a component passing no options; preloadObservable is first-caller-wins, so the second one silently gets the wrong data shape.
  • This is not a new class of problem: on main, {} and { idField: 'undefined' } collide the same way (I verified both against the emulator). This change fixes that pair while creating the 'none' pair, so net exposure is unchanged.

An encoding that cannot collide with any real field name would close the class for good, for example idField=${JSON.stringify(idField)}: idField=undefined for the default, idField="none" (with quotes) for the literal string. Not blocking; happy to leave the simple format if you prefer.

const observable = docData(ref, { idField });

return useObservable(observableId, observable, options) as ObservableStatus<T>;
Expand All @@ -72,9 +72,9 @@ export function useFirestoreDocData<T = unknown>(ref: DocumentReference<T>, opti
* Get a Firestore document, unwrap the document into a plain object, and don't subscribe to changes
*/
export function useFirestoreDocDataOnce<T = unknown>(ref: DocumentReference<T>, options?: ReactFireOptions<T>): ObservableStatus<T | undefined> {
const idField = options ? checkIdField(options) : 'NO_ID_FIELD';
const idField = options ? checkIdField(options) : undefined;

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

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

This ternary expression is redundant. The checkIdField function already handles cases where options is undefined by returning undefined. You can simplify this to a direct call to checkIdField(options).

Suggested change
const idField = options ? checkIdField(options) : undefined;
const idField = checkIdField(options);

@tyler-reitz tyler-reitz Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Declining: the ternary guard at the call site is intentional. checkIdField and checkOptions are typed to expect a real ReactFireOptions object, and the hooks handle the no-options case before calling them. Widening the signatures to accept undefined would require options: ReactFireOptions | undefined since field follows as a required parameter, which isn't a meaningful improvement. The current shape keeps the function signatures strict.


const observableId = `firestore:docDataOnce:${ref.firestore.app.name}:${ref.path}:idField=${idField}`;
const observableId = `firestore:docDataOnce:${ref.firestore.app.name}:${ref.path}:idField=${idField ?? 'none'}`;
const observable$ = docData(ref, { idField }).pipe(first());

return useObservable(observableId, observable$, options) as ObservableStatus<T>;
Expand All @@ -94,8 +94,8 @@ export function useFirestoreCollection<T = DocumentData>(query: FirestoreQuery<T
* Subscribe to a Firestore collection and unwrap the snapshot into an array.
*/
export function useFirestoreCollectionData<T = DocumentData>(query: FirestoreQuery<T>, options?: ReactFireOptions<T[]>): ObservableStatus<T[]> {
const idField = options ? checkIdField(options) : 'NO_ID_FIELD';
const observableId = `firestore:collectionData:${getUniqueIdForFirestoreQuery(query)}:idField=${idField}`;
const idField = options ? checkIdField(options) : undefined;

@gemini-code-assist gemini-code-assist Bot Jul 16, 2026

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

This ternary expression is redundant. The checkIdField function already handles cases where options is undefined by returning undefined. You can simplify this to a direct call to checkIdField(options).

Suggested change
const idField = options ? checkIdField(options) : undefined;
const idField = checkIdField(options);

@tyler-reitz tyler-reitz Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Declining: the ternary guard at the call site is intentional. checkIdField and checkOptions are typed to expect a real ReactFireOptions object, and the hooks handle the no-options case before calling them. Widening the signatures to accept undefined would require options: ReactFireOptions | undefined since field follows as a required parameter, which isn't a meaningful improvement. The current shape keeps the function signatures strict.

const observableId = `firestore:collectionData:${getUniqueIdForFirestoreQuery(query)}:idField=${idField ?? 'none'}`;
const observable$ = collectionData(query, { idField });

return useObservable(observableId, observable$, options);
Expand Down
Loading