reactfire icon indicating copy to clipboard operation
reactfire copied to clipboard

Type of `ObservableStatus::data` should include `undefined`

Open wmadden opened this issue 3 years ago • 1 comments

Version info

React:

Firebase:

ReactFire: 4.2.2

Other (e.g. Node, browser, operating system) (if applicable):

Test case

const myDoc = useFirestoreDoc(myRef);
console.log(myDoc.data.data().anyProperty); // <- will throw an error accessing "data.data()" since it

Steps to reproduce

Description

The Firestore access hooks, e.g. useFirestoreDoc(), useFirestoreCollection() etc, all return an ObservableStatus object. The ObservableStatus interface guarantees the presence of data, which is untrue, since it will initially be undefined (unless using suspense).

Currently, ObservableStatus is defined as follows:

export interface ObservableStatus<T> {
  status: 'loading' | 'error' | 'success';
  hasEmitted: boolean; // has received at least one value
  isComplete: boolean;
  data: T;
  error: Error | undefined;
  firstValuePromise: Promise<void>;
}

Expected behavior

const myDoc = useFirestoreDoc(myRef);
console.log(myDoc.data.data().anyProperty); // <- will throw an error accessing "data.data()" since it
//                                ^^^^^^^
// Type Error: data may be undefined

I expect the types to indicate that Observable::data is either T | undefined.

Actual behavior

The type system does not warn you when accessing properties on data while it's undefined.

At run time it throws an error attempting to access properties on undefined.

wmadden avatar Jan 12 '23 15:01 wmadden

Agree. I've always found the inaccurate types confusing. It leads to error prone code, since the result of these hooks is briefly undefined before they return valid values.

devth avatar Jun 03 '23 21:06 devth