Allow `useFirestoreDoc` `DocumentReference` arguments to be nullable

[ky28059]

react-firebase-hooks allows the document reference to useDocument to be nullable and just returns undefined if it is. This was useful for paths relying on nullable info, like auth.currentUser.uid:

const [snapshot, loading, error] = useDocument(auth.currentUser && firestore.doc(`users/${auth.currentUser.uid}`));

where if the user were not signed in there wouldn't be an error thrown since useDocument would just return undefined. It would be nice if a similar feature were added to reactfire, as

const { status, data: firebaseDoc } = useFirestoreDoc(auth.currentUser && doc(firestore, 'users', auth.currentUser.uid));

won't work as the type of ref is DocumentReference, not DocumentReference | null,

const { status, data: firebaseDoc } = useFirestoreDoc(doc(firestore, 'users', auth.currentUser?.uid));

will throw an error when the user is not signed in (as the path will become invalid), and

if (auth.currentUser) {
    const { status, data: firebaseDoc } = useFirestoreDoc(doc(firestore, 'users', auth.currentUser.uid));
}

violates the rules of hooks. Currently relying on a rather abhorrent workaround to resolve this and it would be ideal if reactfire could support this behavior natively.

[epodol]

@jhuleatt If you would like, you can assign this to me. I would be happy to implement this. I was thinking this could be an option like: allowNullReference. This would prevent breaking anyone using this feature to their advantage, and also allow people to enable this functionality.

Thank you!

[jhuleatt]

Hey @ky28059 and @epodol, this request has come up a couple of times (https://github.com/FirebaseExtended/reactfire/issues/178#issuecomment-550503600, https://github.com/FirebaseExtended/reactfire/issues/166#issuecomment-544642338).

My opinion is that this would create an opportunity for subtle bugs, and is better handled by breaking things out into separate components (render one component if someone isn't signed in, render a different one that does a Firestore query once you know who the user is).

I wrote out an in-depth response here (note that it's an old version of ReactFire): https://github.com/FirebaseExtended/reactfire/issues/178#issuecomment-551283156. Please take a look at that thread. If you still disagree and can provide a specific code sample that shows why breaking out into different components is impractical, I'd be open to discussing further.

[epodol]

Hey @ky28059 and @epodol, this request has come up a couple of times (#178 (comment), #166 (comment)).

My opinion is that this would create an opportunity for subtle bugs, and is better handled by breaking things out into separate components (render one component if someone isn't signed in, render a different one that does a Firestore query once you know who the user is).

I wrote out an in-depth response here (note that it's an old version of ReactFire): #178 (comment). Please take a look at that thread. If you still disagree and can provide a specific code sample that shows why breaking out into different components is impractical, I'd be open to discussing further.

This actually makes a lot of sense, and I agree with you now. Thank you for your explanation!

[hubertkuoch]

Hi there - thanks for the very helpful lib @jhuleatt.

Just jumping in as there is actually a use-case where breaking into small components might not be convenient. This happens whenever you want to feed a component with remote default values if they exist, I think it is actually a quite common use-case.

Just to illustrate simply this use-case, say we have a public contact form page.

• If the user is anonymous : inputs are left blank (normal behaviour)
• If the user is logged in, inputs are filled with the logged-in user information automatically as default values, fetched from database.

In this very case, breaking into small components might not be very practical as we need to create dedicated wrapper component only to fetch the data and populate the default values.

const Form = ({defaultValue}:{defaultValue?: User}) => {
    return (
        <form>
            <input defaultValue={defaultValue.name} disabled={!!defaultValue}/>
        <form>
    )
}

const FormWithDefaultValue = ({uid}:{uid: string}) => {
    const userRef = useFirestore().collection('users').doc(uid)

    const { data } = useFirestoreDoc(userRef);

    return (
        <Form defaultValue={data}>
    )
}

const ContactPage = () => {
    const {data: signInCheckResult} = useSigninCheck()
    
    if (signInCheckResult.signedIn) {
        return <FormWithDefaultValue/>
    } else {
        return <Form/>
    }
}

Happy to get your views, thanks.

(Disclaimer: I might not yet very familiar with ReactFire, please forgive me if I miss understood its usage philosophy).

[sarelp]

Hi @jhuleatt , I have read your comments in #178. I do however believe that there is still a necessity for this.

Specifically, in our codebase, we have a number of documents referencing each other i.e.

type Page = {
   id: string,
   theme: DocumentReference(Theme) | null,
   view?: DocumentReference(View) | null,
   user?: DocumentReference(User) | null,
}

Rendering requires getting a number of documents which is not easily broken down into subcomponents. In our case it would require nesting a number of levels deep.

Having a nullable or undefined argument will greatly simplify things. We do have undefined and null in javascript to distinguish to enable the API to distinguish between not yet resolved and not available.

useFirestoreDoc(null) can still return a doc but with doc.data() == null and doc.exists() == false

[Gnadhi]

Any updates on this issue as Im currently facing a similar problem

[jasonlav]

Also facing this same issue. In my situation, the user id is required to make the query. Breaking apart things into components for the sake of this current limitation would be incredibly inefficient.

[dmvvilela]

Any updates? I am almost changing the library cause this does not make sense to me.. I had a lot of places already that rules of hooks are being broken cause I need conditionals..

For example in Next.js I can't init performance cause I need to check if it's on the browser and everything is a hook and the rules are broken..

So, I have a few ideas on how to fix this:

1. Do not do stuff on hook call, but get the query function so it can be used conditionally.
2. Add a skip parameter like Apollo does with graphql, so it becomes conditional in a way.
3. Why it is a hook? Is it an actual necessity or could be a simple query function and nobody would have this problems anymore..

For now to be honest I will have to change library for Firebase..

[KatFishSnake]

Unfortunately changing the library as well as this is a deal breaker

[devth]

Current behavior leads to code like this all throughout my codebase:

  const { data: networkMemberships } =
    useFirestoreCollectionData<NetworkMembership>(
      profile
        ? query(
            collection(network.ref, "members").withConverter(
              converter<NetworkMembership>()
            ),
            where("profileRef", "==", profile.ref)
          )
        : collection(network.ref, "noop").withConverter(
            converter<NetworkMembership>()
          ),
      { idField: "id" }
    );

😢

I would love for this to be more like:

  const { data: networkMemberships } =
    useFirestoreCollectionData<NetworkMembership>(
      profile &&
        query(
          collection(network.ref, "members").withConverter(
            converter<NetworkMembership>()
          ),
          where("profileRef", "==", profile.ref)
        )
    );

[fvanwijk]

In react-query there is enabled: true which works perfectly for all these use cases. So with reactfire you would do:

const { data } = useFirestoreDocData<User>(doc('users/' + userId), { enabled: !!userId }));

The type of data would be User | null.

Or with collections:

const { data } = useFirestoreCollectionData<User[]>(query(collection('users')), { enabled: !!userId });

The type of data would be User[] | null.