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

[tyler-reitz]

Summary

• useFirestoreDocData, useFirestoreDocDataOnce, useFirestoreCollectionData, useDatabaseObjectData, and useDatabaseListData were passing the string 'NO_ID_FIELD' to rxfire's keyField option when no idField was provided by the caller
• This caused rxfire to add a property literally named "NO_ID_FIELD" (with the document ID as its value) to every returned document, polluting user data with an internal sentinel
• Fix passes undefined instead, which rxfire already handles correctly by omitting the ID field entirely
• Cache keys updated to use 'none' as the no-idField sentinel (replacing undefined, which stringifies to "undefined" in template literals and would make cache keys ambiguous)

Semver note

This is a bug fix: NO_ID_FIELD appearing in returned data was never documented or intentional behavior. Code reading result.NO_ID_FIELD would break, but that code was relying on a bug. Semver call (patch vs. minor/major) left to Jeff, and this may ship alongside #735.

Test plan

• TypeScript: clean
• Firestore emulator tests: 11/11 passing
• Covered by existing emulator test suite for useFirestoreCollectionData, useFirestoreDocData, and database equivalents

Fixes #295

[gemini-code-assist]

Code Review

This pull request updates the idField initialization logic in src/database.tsx and src/firestore.tsx to use undefined instead of a string literal, and updates the observableId generation to use the nullish coalescing operator for better readability. The reviewer suggests further simplifying the idField assignment by removing redundant ternary operators, as the checkIdField function already handles undefined inputs appropriately.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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]

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.

[armando-navarro]

I reproduced the problem on main before reading the diff, and the fix checks out end to end. One request before merge, then some notes you can take or leave.

What I verified

• On main, emulator tests asserting exact equality with the written data fail for all five hooks, with "NO_ID_FIELD": "<id>" injected (firestore doc, doc-once, and collection data; RTDB object and list data).
• The same tests pass on this branch, and the full firestore and database suites show no regressions.
• In rxfire 6.1.0's source, a falsy idField/keyField adds nothing. The edge cases are safe too: missing docs, primitives, and non-object converter output never got the sentinel in the first place.
• Lint output is identical to main's, and tsc is clean.

Request: commit regression tests for the no-options path

Unless I've missed something, the changed path has no coverage today:

• Every existing test of these five hooks passes idField: 'id' explicitly.
• useDatabaseObjectData and useDatabaseListData have no tests at all.
• So CI passes identically with or without this fix, and the sentinel could quietly come back.

The tests I used for verification are in the "Example test code" section at the end of this review; feel free to take them as-is, adapt them, or replace them with your own. One caution: if they go in as new files, two test files sharing the RTDB emulator will wipe each other's data (each file's afterEach clears the whole database, and vitest runs files in parallel workers), so folding them into firestore.test.tsx and database.test.tsx is safer. With tests in, the "Covered by existing emulator test suite" line in the description would be worth pointing at them.

Description notes (only because the squash body becomes the changelog)

• As far as I can tell, the sentinel only appeared when the options argument was omitted entirely. Passing {} or { initialData } already reached rxfire with undefined and returned clean data. So this PR also unifies the omitted-options vs {} inconsistency, which seems worth claiming credit for.
• "every returned document" could narrow to object data: missing docs, primitives, and non-object converter results never got the sentinel.

Happy to be corrected if you saw the sentinel in a case I've missed.

Semver

I agree with leaving the call to Jeff. One more data point for him: with no idField, docData previously spread results into a fresh plain object, which flattened data-converter class instances; with this change they keep their prototype. Consumer-visible in two ways, so minor alongside #735 seems right to me.

The fix itself looks exactly right, and everything above is additive. Feel free to push back if you feel it should be approved as is.

Example test code

For test/firestore.test.tsx, inside the existing useFirestoreDocData and useFirestoreCollectionData describes:

it('returns exactly the stored data when no options are provided', async () => {
  const mockData = { a: 'hello' };
  const ref = doc(collection(db, randomString()), randomString());
  await setDoc(ref, mockData);

  const { result } = renderHook(() => useFirestoreDocData(ref), { wrapper: Provider });
  await waitFor(() => expect(result.current.status).toEqual('success'));

  expect(result.current.data).toEqual(mockData);
});

it('returns exactly the stored data when no options are provided', async () => {
  const mockData = { c: 'collection' };
  const collectionRef = collection(db, randomString());
  await setDoc(doc(collectionRef, randomString()), mockData);

  const { result } = renderHook(() => useFirestoreCollectionData(collectionRef), { wrapper: Provider });
  await waitFor(() => expect(result.current.status).toEqual('success'));

  expect(result.current.data).toEqual([mockData]);
});

For test/database.test.tsx, new describes (imports needed: useDatabaseObjectData, useDatabaseListData, push):

describe('useDatabaseObjectData', () => {
  it('returns exactly the stored data when no options are provided', async () => {
    const mockData = { a: 'hello' };
    const objectRef = ref(database, randomString());
    await set(objectRef, mockData);

    const { result } = renderHook(() => useDatabaseObjectData(objectRef), { wrapper: Provider });
    await waitFor(() => expect(result.current.status).toEqual('success'));

    expect(result.current.data).toEqual(mockData);
  });
});

describe('useDatabaseListData', () => {
  it('returns exactly the stored data when no options are provided', async () => {
    const mockData = { b: 'world' };
    const listRef = ref(database, randomString());
    await push(listRef, mockData);

    const { result } = renderHook(() => useDatabaseListData(listRef), { wrapper: Provider });
    await waitFor(() => expect(result.current.status).toEqual('success'));

    expect(result.current.data).toEqual([mockData]);
  });
});

(useFirestoreDocDataOnce without options is also uncovered; same shape as the DocData test with useFirestoreDocDataOnce if you want the full set. My verification ran all five.)

[armando-navarro]

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.