useCollection for downstream data instead of .get()/useEffect()

jamesncox commented 3 years ago

Description

IMPORTANT! Only merge this PR once issue 8 (#9) has been merged! I created this branch off that branch. Next time I'll know to create a branch off main.

This PR completely removes the REST-style GET request to the Firebase API, and instead takes advantage of the Firebase hook, useCollection, in both List.js and AddItem.js. After discussing the issue with @skylerwshaw, I have a much better understanding how Firebase useCollection hook works, providing a constant down-stream of data. When we had the fetch inside our useEffect(), we were not leveraging the power of Firebase via the useCollection hook.

Related Issue

closes #23

Acceptance Criteria

This PR goes a different direction than the original issue. Having the connection to Firebase in App.js makes a lot of sense. But it causes more headaches/bugs than it solves, because before the token is fully resolved in its useEffect() the useCollection hook runs with an empty token.

So we definitely want to leverage useCollection but we need to have a persisted token first. And because the way we route between either List.js OR AddItem.js, the connection to the database via useCollection will never be called simultaneously between the two components.

@skylerwshaw helped me liken it to how Redux provides a central store of state for the whole app to access. Firestore is kind of like our central store, and we're reading from it similarly how you might request data from Redux central store in multiple components.

After all those considerations, I don't see it as a negative having useCollection called in two different components.

Type of Changes

	Type
	:bug: Bug fix
	:sparkles: New feature
✓	:hammer: Refactoring
	:100: Add tests
	:link: Update dependencies
	:scroll: Docs

Updates

Remove the useEffect() containg our .get() request to Firestore.
Add useCollection to both List.js and AddItem.js.

Before

In App.js:

 useEffect(() => {
    if (token) {
      db.collection(token)
        .get()
        .then((querySnapshot) => {
          const listItemData = [];

          querySnapshot.forEach((doc) => {
            listItemData.push(doc.data());
          });
          setListItems(listItemData);
        });
    }
  }, [token]);

After

Added to List.js and AddItem.js:

 const [listItems, loading, error] = useCollection(db.collection(token), {
    snapshotListenOptions: { includeMetadataChanges: true },
 });

Testing Steps / QA Criteria

CD into tcl-23-smart-shopping-list
Run git checkout main
Run git pull
Run git checkout jc-use-collection-instead-of-get
Run npm install
Run npm start
Notice that everything still works great!

github-actions[bot] commented 3 years ago

Visit the preview URL for this PR (updated for commit db6ca1e):

https://tcl-23-shopping-list--pr24-jc-use-collection-in-fcbct6t3.web.app

_{(expires Sun, 09 May 2021 21:13:07 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

jssckbl commented 3 years ago

Thank you for doing this, James! Also, I appreciate the thorough explanation of the process you went through with this issue, and sharing the comparison of Redux to our use of Firestore in this instance.

the-collab-lab / tcl-23-smart-shopping-list