FirebaseExtended / reactfire

Hooks, Context Providers, and Components that make it easy to interact with Firebase.
https://firebaseopensource.com/projects/firebaseextended/reactfire/
MIT License
3.5k stars 400 forks source link

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

Open wmadden opened 1 year ago

wmadden commented 1 year ago

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.

devth commented 1 year ago

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.