Handle undefined gracefully in hooks?

[davepwsmith]

I'm not sure if this is a deliberate choice to force me to think harder about breaking my app down into teeny-tiny pieces, but for discussion anyway:

The nature of hooks is that they can't be called conditionally. But if I am using a field from another document which should contain a document reference to feed into useFirebaseDocData or some-such, then by the nature of a schemaless database that reference might not actually be there. This means that in some cases, it is hard to guarantee that what I am trying to hand to the hook isn't undefined.

react-firebase-hooks handles this by (as far as I can tell) returning undefined when you feed it undefined - which means that you can handle this lack of data from within the component. As it stands, reactfire's hooks just throw their arms up in the air (at least in typescript-land) which means that instead I need to split out every component that relies on data from more than one place into its own thing, and then render the whole component conditionally based on the availability of the data. It may seem like the right approach, but in practice it seems to mean lots of small, not at all re-usable components end up getting made.

Your thoughts?

[jhuleatt]

Hi @davepwsmith!

It looks like you caught a bug in ReactFire. It turns out that the library that underpins ReactFire, RxFire, doesn't handle the undefined case correctly. I've filed an issue with more detail here. In cases where it should return null or undefined, it's actually returning {}.

[davepwsmith]

So - that definitely looks like a bug in rxfire... but I'm not sure still that hooks shouldn't handle this a little differently?

When you use the firebase JS SDK without hooks (or even in your own custom hooks) because you aren't using hooks, you can always wrap things in conditionals. So for example if I want to get a document based on the users UID I can make that fetch conditional on the user object existing - pesudocode:

const user = firebase.auth().currentUser
if(user && user.uid) {
    return {...user, ...getFirestoreDataByUid(user.uid)}
} else { return user }

But because you can't call hooks conditionally, you can't do a similar thing with react hooks, so if I want to get a user (useUser()) and then fetch their profile (useFirestoreDocData()) I might end up actually passing undefined into the hook, resulting in various contortions e.g.:

// This could be null, in which case user.uid is undefined 
const user = useUser()

// - or in the case of this hook even 'unknown', so we need a typeguard

function isUser(user: any): user is firebase.User {
  return user.uid && user.uid !== null;
}

// So here, you have to guard against the possibility of it being undefined or null, 
// because typescript makes you (and it's the right thing to do)...
const userDoc = isUser(user) 
    ? firestore.collection('users').doc(user.uid) 
// But you have to pass in a document reference, even if it is empty, 
// because the hook demands it
    : firestore.collection('users').doc()

// And now I can use my document hook, but I may be looking up a 
//nonexistent document
const doc = useFirestoreDoc(userDoc)

// So I have to check if it exists, and add the data to the ID... 
// and if it doesn't exist I have a document reference that I don't want
const userData = doc.exists ? {...doc.data() as UserData, id: doc.id}: null

react-firebase-hooks - which doesn't use rxfire I don't think, so there might be some constraints here that I don't understand around observables etc. - seems to get around this by returning null when things don't exist, and allowing you to pass null into other hooks and return null. It sounds silly, but it means that instead of the above you get something like:

const user = useUser()
// returns null because user doesn't exist

const userDoc = user ? firestore.collection('users').doc(user.uid)  : null
// Because I can pass null into the document hook, I don't have to 
// have empty document references hanging around... so userDoc is NULL
const useDocument(userDoc)
// also returns null - so I can easily see what exists or doesn't and act accordingly

This seems to me like a graceful way of handling the "hooks can't be called conditionally" case - but equally, perhaps others would see it as circumventing the way it's all supposed to work?

[ysamlan]

This doesn't really solve the deeper issue but I wrote this helper that at least avoids having to do those empty DB object lookups by just subbing in an Observable emitting undefined and wrapping it in a proper Reactfire-shaped holder:

function useUndefinedDatabaseObjectData(): ObservableStatus<undefined> {
  return useObservable<undefined>("$undefinedobservable", of(undefined), {
    suspense: true, // if using suspense
  });
}

Besides skipping an empty database lookup and just seeming less ooky, this is helpful because in my case (objects in realtime DB with security rules) I can't just pass in a blank ID -- it ends up trying to retrieve the root collection (which is against the rules) or a nonexistent item (which the user hasn't been granted access to and will also fail).

That said, it's still not very elegant to use; maybe there's a way to wrap/compose hooks for a chained-conditional-database-lookups scenario, but I haven't figured that out yet.

[jhuleatt]

This is definitely a tricky problem that can show up all over an app, like when a field in a document points to a storage link that we then want to call useStorageDownloadURL on, etc.

The null passing solution gets around React's warnings, but in practice it seems like any case where you'd pass null to a hook might be better solved by the hook throwing an error (potentially to be caught by an Error Boundary) because it is a case where something unexpected happened and we went down a branch of the render tree that we shouldn't actually be in (in your example case, because the user never logged in).

ReactFire solves the auth case with the AuthCheck component. Its children are guaranteed that any call to useUser will return a valid user. If a user isn't signed in, it renders the component passed to its fallback prop instead.

Having more conditional components like this could be the way to solve null checking for other cases. However, the problem is that we as the developer just have to "know" that child components can safely get a user (or whatever else), and that could open us up to bugs if, for example, we accidentally remove the AuthCheck parent during a refactor. But I think error boundaries are the right answer for that too.

TL;DR I'm not totally sure what the right solution is, but passing null and not treating it like an error feels to me more like a hack than the right thing to do. But I'm definitely open to more discussion on this.

[davepwsmith]

Standing aside from the Auth issue (and yes, AuthCheck is useful, but typescript still complains that user can be unknown, as I think you alluded to - I wonder if there is some way to fix that? I also think that getting a user and a corresponding firebase document is probably such a common use-case as to warrant a hook of its own!):

I guess for me, the issue is that in a schemaless database like firestore you might not have a field, and since you can't call hooks conditionally, this does not seem like it would necessarily always be an "error"? I have probably just got used to how something else works and now will have to get used to this!

If documents can be different, then hooks can thus return different information... and in addition to this, sometimes you will want to wait for the result of one hook to pass into another. Perhaps I am just using hooks wrong! As far as I can tell this basically precludes a lot of cases where you might want to use multiple dependant hooks in a component. Perhaps the way around this is to either split components down further or to write custom hooks which compose the various hooks from this package, or using the rxfire/useObservable combo?

rxfire makes a big deal of how it makes it easy to combine data from different firestore documents (https://firebase.googleblog.com/2018/09/introducing-rxfire-easy-async-firebase.html) and other firebase components, so this was kind of something I was expecting to be straightforward in a library built on top of it.

It's somewhere between possible and very likely though, that the reason I'm not finding it straightforward is because I've misunderstood something basic, so if between us we can find an acceptable pattern for how this should be tackled that you would be happy to recommend to users of your library, perhaps I will help to document it!