feature: hooks

[peternoordijk]

Warning: This PR also contains #40 - it might be better to check and merge that one first and then separately check this PR after.

This pull request adds hooks! 🎉

import { useFirestore, query } from 'react-firestore';

const MyComponent = () => {
  // Get the instance of the firestore
  const firestore = useFirestore();

  // Or use the query builder to retrieve data right away. Note that the actual hook is 
  // called once you call .useSnapshot()
  const { data, isLoading, snapshot, error } = query()
    .collection('users')
    .where('foo', '==', 'bar')
    .limit(5)
    .useSnapshot();
}; 

One small note: unit tests for useEffect currently don't work that good yet. I wasn't able to test this using enzyme. I didn't manage to get a unit test working to check that the hook responds good to changes in the query. However I did test the functionality in the browser, and that works as expected. I still managed to get the coverage to 100%.

[codecov[bot]]

Codecov Report

Merging #41 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #41   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      7    +1     
  Lines          88    130   +42     
  Branches       18     25    +7     
=====================================
+ Hits           88    130   +42

Impacted Files	Coverage Δ
src/withFirestore.js	100% <100%> (ø)	:arrow_up:
src/FirestoreDocument.js	100% <100%> (ø)	:arrow_up:
src/FirestoreProvider.js	100% <100%> (ø)	:arrow_up:
src/useFirestore.js	100% <100%> (ø)
src/Firestore.js	100% <100%> (ø)	:arrow_up:
src/FirestoreCollection.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9b00fdf...521391f. Read the comment docs.

[benjick]

@green-arrow Any interest in adding maintainers to this project?

[green-arrow]

@peternoordijk - Thanks for looking into hooks! Could you explain a bit about your decision to use method chaining as a way to build a query?

I've been thinking about hooks since they were announced, and I originally had imagined hooks that mirrored the components (useFirestoreCollection and useFirestoreDocument). My thought was that the first arg would be the name / id, and the second would be an options object that would correspond directly to the other props available on the FirestoreCollection and FirestoreDocument components.

I'm a bit worried that the method chaining might add some more complexity for users and is not quite as explicit as something like useFirestoreCollection. Also, having the onSnapshot method required makes the user have some knowledge of the firestore API, while my initial goal for this library was to abstract away some of that by just providing data directly to the user and not via snapshot.

Let me know what you think!

[peternoordijk]

@green-arrow Thanks for looking into the PR, and I understand your concerns. The reason why I chose for a builder pattern is to allow reusability like so:

const MyComponent = () => {
  const userDocument = query().collection('users')
    .doc('some-id');
  
  const userBooksResult = userDocument.collection('books').useSnapshot();
  const userTasksResult = userDocument.collection('tasks').useSnapshot();
}

Notice that I called the last method useSnapshot because that's what's actually triggering a hook to be built. However I'm aware that terminology could be improved in this PR. I guess query works better when called fireQuery or something. useSnapshot might be changed into something else like useResult or something.

Also there's another feature in this PR to 'disable' the query (I think that also has to move to the useSnapshot method now that I see this code again. Basically this allows you to make conditional queries based on previous results which will only execute once the previous results are loaded. I will later today create another commit with this little change. There's also a small bugfix I have to make to this PR (newBuilder. isDoc is not working correctly with sub collections).

By the way I totally see your point with useFirestoreCollection and useFirestoreDocument. I guess we can just add those options as well alongside with the query builder, what do you think? I'd be happy to work on this.

[peternoordijk]

@green-arrow I just updated this PR as well as the first one. I think I should clarify one thing reading your message from before.

Right now the api looks like this:

import { useFirestore, fireQuery } from 'react-firestore';

const MyComponent = () => {
  // Same format like render props below:
  const { data, isLoading, snapshot, error } = fireQuery()
    .collection('users')
    .where('foo', '==', 'bar')
    .limit(5)
    .useResult();
};

Note that the data which you get back is not (only) the actual snapshot. It returns the same signature as the data in the renderprops: isLoading, error, data and the snapshot.

I didn't write the useFirestoreCollection yet... There's also an issue I found when using the 'query hook' in the last weeks. The thing is that the arguments and everything are being converted into a dependency array for the effect hook. However, the dependency array needs to have the same length at all times.

This for example will be impossible:

const MyComponent = () => {
  // This doesn't work whenever the condition changes
  let query = fireQuery().collection('users');
  if (somecondition) {
    query = query.where(...);
  }
};

I do have a workaround for this by adding a "dummy" method to call so that the dependencies will at least have the same length.

const MyComponent = () => {
  // This heck works
  let query = fireQuery().collection('users');
  if (somecondition) {
    query = query.where('a', '==', 123);
  } else {
    query = query._(1, 2, 3);
  }
};

However this really is an ugly hack. I think we also have to come up with a more elegant answer to this, because this same issue will appear in useFirestoreCollection (if we allow a dynamic filter array for example). At least there's other people with the same problem.

I kind of want to fix this before going any further adding more code to this. Do you have any ideas on this one?

[gaearon]

https://github.com/facebook/react/issues/14476#issuecomment-471199055

[gaearon]

(I would note that foo.bar.useSomething() is not very idiomatic and won't be checked by the Rules of Hooks linter. It's also super easy to miss that this is a Hook call. I would look into consider a different API (e.g. chain it but then have useQuery(chain) or similar).

[green-arrow]

@peternoordijk - Thanks for the explanation and sorry again for the long delay.

I agree with @gaearon regarding the chaining. This feels like more of an advanced use case to me, so I think we could stand to make the usage a tad more complex. This is what I'm thinking:

const firestore = useFirestore();
const query = firestore.collection('users');

// Do stuff with query like you normally would when using it directly.
// Execute `where` clauses, `orderBy`, etc.

const const { data, isLoading, snapshot, error } = useFirestoreQuery(query);

For use cases where the query can change due to props or other state, the consumer would be required to pass in a dependency array to useFirestoreQuery, just like useEffect.

const { userId } = props;
const firestore = useFirestore();
const query = firestore.collection('users').where('userId', '==', userId);
const const { data, isLoading, snapshot, error } = useFirestoreQuery(query, [userId]);

What are your thoughts on an API like this?

[peternoordijk]

@gaearon Hi Dan! Great to see you on this PR as well.

@green-arrow The api looks good I think, however the query on the version that I made wasn't a direct call to the actual firebase API. Instead it just store all the calls so that it will only call the actual API once anything changes.

With that being said, the API that you proposed would look like this:

const firestore = useFirestore();
// useQuery does NOT return the same object as the firebase object. Instead it returns some
// builder which we can later compare to previous builders
const query = useQuery().collection('users').where(...);

// Do stuff with query like you normally would when using it directly.
// Execute `where` clauses, `orderBy`, etc.

const const { data, isLoading, snapshot, error } = useFirestoreQuery(query);

This means we also don't have to provide the dependencies since we already have all the data available inside the builder objects. For big queries you would otherwise basically have to define the same query twice (the second time being the dependencies).

What do you think of this idea?