Q. Should hooks return an Object or an Array?

[tograd]

makes sense: snapshot.data(); snapshot.id

doesn't: value.data(); value.id

Just food for though. I see you pushed 1.0 so I doubt you wanna make breaking changes. I can just wrap anyway

[chrisbianca]

@tograd I spent a long time debating how best to do this, and settled on value although I agree, it is not ideal in all situations. I am open to changing if there is a feeling that it should be, but as you say it would need a major version given the breaking change.

The other approach I had considered was to use array destructuring for the return from each of the useXXX methods which is the approach that the official hooks use for multiple returned values. This means that naming does not matter so much.

For example:

const [user, initialising] = useAuthState(firebase.auth());
const [snapshot, loading, error] = useDocument(firebase.firestore().doc('/test/path'));

I'd be interested in your thoughts?

[tograd]

You are right that arrays fits into the general hook "scheme" and typescript supports tuples well.

But on the other hand I don't like destructuring arrays, and we lose the dot access of properties and ability to "...rest" properties. I mean arrays destruct leans to clean syntax for renaming properties, but it also FORCES you destruct even when you perhaps don't want to. Plus, basically nothing outside the "standard lib" of hooks actually return arrays, I guess for the reasons stated above or other good reasons.

I've given this some additional thought and I think perhaps value is the moderate ideal for all situations after all. I personally for my own trivial reasons can just do the silly re-export below, and get whatever stupid property names I want, and in niche-cases I can just rename directly when destructuring from useCollection()/useDocument() anyway.

import {
  useCollection as _useCollection,
  CollectionHook as _CollectionHook,
} from "react-firebase-hooks/firestore";

export interface CollectionHook {
  collection: _CollectionHook["value"],
  loading: _CollectionHook["loading"],
  error?: _CollectionHook["error"],
}

export const useCollection = (...args: Parameters<typeof _useCollection>): CollectionHook => {
  const { value, ...rest } = _useCollection(...args);

  return { collection: value, ...rest };
};

(I want to re-export your functions anyway so)

[chrisbianca]

I'm still torn, so I'm going to leave this open for the time being.

I can understand your rationale, but equally, I wonder whether the "non standard" hook libs return objects because that's what users and developers are used to. Array destructuring is a relatively new concept and certainly hooks are one of the first mainstream implementations that I've seen. It could be that over time, it becomes more common to return arrays for this sort of thing.

I'm also curious to understand what you meant by losing dot access of properties or the ability to ...rest properties?

With array destructuring, you could still do the following:

const [user, ...rest] = useAuthState(firebase.auth());
const [{ displayName, email, ...rest }, initialising] = useAuthState(firebase.auth());

[nmaves]

About time I weigh in and give some feedback on this. I agree 💯 that we should return an array.

I have multiple components that use more than one collection and therefore use more than one useCollectionData() (thanks @chrisbianca for adding that function).

Given the two options below I choose arrays. While the use of object destructing helps it is not all that readable. We are talking about a React Hook library here. If you can't understand array destructing you were lost at useState.

const [people, loadingPeople] = useCollectionData(peopleQuery, null, 'd')
const [places, loadingPlaces] = useCollectionData(placesQuery, null, 'id')

const { value: people, loading: peopleLoading } = useCollectionData(peopleQuery, null, 'd')
const {value: places, loading: loadingPlaces }= useCollectionData(placesQuery, null, 'id')

See @chrisbianca that null is just in my way 😈

[cdock1029]

Been messing with some ideas in my own hooks repo: https://github.com/cdock1029/fire-hooks

I like getting rid of the loading variables, seems cleaner.

const [people, error] = useCollectionData(peopleQuery, {id, options})

and just have people be: T[] | undefined if it's undefined, it's still loading. If it has no data, it's an empty array.

For Document: T | undefined | null

Optional arguments all get put in an object, because optional positional arguments lead to no good scenario if using mostly one or the other, and don't have names so are confusing when looking at code later.

One other idea I'm trying is to throw the error inside the hooks. Then have an ErrorBoundary somewhere. Still not sure... but something to play with.

So then we'd have:

const people = useCollectionData(peopleQuery)

I see how you're accounting for query/ref updates and that's pretty nice, checking equality and such. I just did a queryBuilder function and a dependencies array like with other hooks which reruns when they change:

const people = useCollectionData<Person>(() => {
    if (companyId) {
      return firebase
       .collection('companies')
       .doc(companyId)
       .collection('people')
    }
  }, [companyId])

[nmaves]

I tend to lean towards a more verbose syntax. I like reading code like if (loading) over if (!people). I take queues from frameworks like graphql where the loading state is explicit. In the end, there are always 3 states (loading/empty/values) so I vote for a loading variable.

[tograd]

I reject my past qualms about arrays. They are indeed quite neat.

[chrisbianca]

@cdock1029 Thanks for your suggestions.

I tend to agree with @nmaves on being explicit about the loading state. Implying anything is always a risk, particularly given that undefined could then mean either: no query reference has been supplied, or the current query is being loaded.

I like what you've done with the dependencies array, but it does leave scope for the user forgetting to include a variable in the array. Can you see any issues with the approach that's been taken currently where this is handled by the library itself?

The error boundary is an interesting suggestion too - I'll have a think about this and see whether it could be implemented cleanly.

Definitely agree that optional arguments going into a single object makes things much cleaner. I'll make this change for v2 of the library. More on this below.

@nmaves You've convinced me on the arrays. I'd been leaning that way as well, but I'm glad that this appears to be the consensus, so I'll get this changed as part of v2 as well.

[chrisbianca]

Is there any preference to ordering of the returned array? My plan was to do the following:

[value, loading, error]

[nmaves]

I can go with that. Why would we not just throw the error? I think all of the errors that I have run into were not recoverable so I ended up just throwing it.

Here is my wrapper hook.

export const useCollectionData = (query, idField = 'id') => {
  const { error, loading, value } = useCollectionDataRFH(query, null, idField)

  if (error) {
    throw error
  }

  return [value || [], loading]
}

[cmmartin]

+1 for returning an array so we can destructure with multiple hooks without doing this...

const { value: user, loading: userLoading, error: userError }  = useDocument(...)
const { value: widget, loading: widgetLoading, error: widgetError }  = useDocument(...)

I also agree on the order [value, loading, error]

Not convinced we should throw the error though. You always have the option to throw in userland if we don't do it, but it's hard to avoid it if we do. AFAIK you can't call a hook inside a try block

[chrisbianca]

Good news, I've just published v2.0.0-rc.1 which updates React Firebase Hooks to use Arrays rather than Objects, as well as a few other things.

You can see everything that's changed here: https://github.com/CSFrequency/react-firebase-hooks/pull/20

Updated documentation is available here: https://github.com/CSFrequency/react-firebase-hooks/tree/v2.0.x

You can install the Release Candidate by running the following:

NPM: npm install --save react-firebase-hooks@next Yarn: yarn add react-firebase-hooks@next

Feedback on the PR would be greatly appreciated!!

[nmaves]

I started trying this out tonight. Everything is working great so far. If we in the mood for breaking changes I think we should always return an array, even if it is an empty one, from our collection* functions. It allows you to treat data as just that. Not undefined. You should not use the data prop in lieu of the loading state.

[chrisbianca]

@nmaves Thanks for reporting back on your initial testing, I'm glad things are working so far!

I'm not convinced about returning an array for all collection* hooks because I think it would hide one of the possible states. For reference, these are the possibilities:

1. Reference provided, loading error: undefined, loading: true, data: undefined
2. Reference provided, data available: error: undefined, loading: false, data: array
3. Reference provided, error error: undefined, loading: false, data: undefined
4. Reference not provided error: undefined, loading: false, data: undefined

If the collection* hooks always returned an array, then you would not be able to differentiate between 2 and 4.

Now, this may or may not actually be an issue in itself, but I'm not sure that the library should arbitrarily making that decision?

[nmaves]

How is the fourth case not an error?

On Fri, May 17, 2019 at 1:02 AM Chris Bianca notifications@github.com wrote:

@nmaves https://github.com/nmaves Thanks for reporting back on your initial testing, I'm glad things are working so far!

I'm not convinced about returning an array for all collection* hooks because I think it would hide one of the possible states. For reference, these are the possibilities:

1. Reference provided, loading error: undefined, loading: true, data: undefined
2. Reference provided, data available: error: undefined, loading: false, data: array
3. Reference provided, error error: undefined, loading: false, data: undefined
4. Reference not provided error: undefined, loading: false, data: undefined

If the collection* hooks always returned an array, then you would not be able to differentiate between 2 and 4.

Now, this may or may not actually be an issue in itself, but I'm not sure that the library should arbitrarily making that decision?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CSFrequency/react-firebase-hooks/issues/5?email_source=notifications&email_token=AACZZBVLPWNLLLDFFIPAIMDPVZKAZA5CNFSM4GWKJVR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVT5TQA#issuecomment-493345216, or mute the thread https://github.com/notifications/unsubscribe-auth/AACZZBQPBOVOXFHSUGXZP73PVZKAZANCNFSM4GWKJVRQ .

[chrisbianca]

This is a valid scenario if you want to combine hooks together, e.g.

const [user, userLoading] = useAuthState(firebase.auth());
const [favourites, profileLoading] = useCollectionData(user ? firebase.firestore().collection(`user-favourites/${user.uid}`) : undefined);

For some history on this, take a look at https://github.com/CSFrequency/react-firebase-hooks/pull/10

[nmaves]

Based on all of that I have to agree with you.

[chrisbianca]

Great, I'm glad I convinced you :D

[nmaves]

No worries, It appears to be a good way to approach conditional hook rendering when they are dependent on conditional data.

[chrisbianca]

v2.0.0 has been published, thank you all for your feedback in helping to shape the release!

Release notes: https://github.com/CSFrequency/react-firebase-hooks/releases/tag/v2.0.0

[ckeeney]

This should perhaps be a new issue, but since this is the discussion thread I thought I would mention it here.

I recently came across the following pattern that would make the return value behave both like an ordered array and an object, depending on how it is destructured.

https://github.com/i18next/react-i18next/blob/0c589dd03bf57c2bb126a9cb7a959d1dde7422b2/src/useTranslation.js#L81-L87

This has the obvious benefit of making this not a breaking change.

  const ret = [t.t, i18n, ready];
  ret.t = t.t;
  ret.i18n = i18n;
  ret.ready = ready;

  // return hook stuff if ready
  if (ready) return ret;