feat(hooks): hook rework to match existing HOC

[illuminist]

Description

List of changes:

• useFirebaseConnect and useFirestoreConnect is now reworked to be the same as firebaseConnect and firestoreConnect HOC respectively which mean full backward compatible even with exactly same query being use.
  • Also now accept an array for multiple query per hook.
• firebaseConnect and firestoreConnect is now internally use useFirebaseConnect and useFirestoreConnect with full backward compatible and no breaking change(as I can think of).
  • With a bonus: they now also accept single query without an array.
• Rework on relevant skipped unit test to actually do some test.
• Avoid firebase props crashing by pulling firebase/firestore instance from context instead. And also pass the instance to child component props.

Check List

If not relevant to pull request, check off as complete

• Typescript type
• All tests passing
• Docs updated with any changes or examples if applicable
• Added tests to ensure new feature(s) work properly

Relevant Issues

• Also fix fix(firebaseConnect): move hoistStatics to allow static methods to be copied - @jeloagnasin #733

[codecov]

Codecov Report

Merging #734 into v3.0.0-alpha.15 will increase coverage by 3.77%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##           v3.0.0-alpha.15     #734      +/-   ##
===================================================
+ Coverage            82.86%   86.64%   +3.77%     
===================================================
  Files                   30       30              
  Lines                  969      936      -33     
  Branches               177      175       -2     
===================================================
+ Hits                   803      811       +8     
+ Misses                 166      125      -41

[prescottprue]

A number of folks use a list of queries being passed to firebaseConnect and firestoreConnect - can you remind me why it is necessary for useFirebaseConnect and useFirestoreConnect to only accept a single query? Is that for easier memoization of the query?

Awesome to see all the work on this!

[illuminist]

It isn't necessary. I just didn't know how to compare previous props inside useEffect at the time I create these hooks. And I was looking at react-redux on how it was going to implement its hook which quere also unsure at the time. That's why limitation came in. But with this PR, I can compare previous props and fully replicate life cycle function using useEffect. So the hooks work just like HOC counterpart, which mean both hooks and HOCs can accept both single query and array of query now.

const Comp1 = firebaseConnect(['test1', 'test2'])(() => <div />)

is same as

const Comp2 = () => {
  useFirebaseConnect(['test1', 'test2'])
  return <div />
}

Another reason is 80% of time I use only single query inside HOC before, even sometimes I forgot to return a query as an array so it was a bit inconvenience for me.

[illuminist]

Hold on a bit.

I misread react-redux changelogs that they included deps argument on useSelector but thay added and removed in the version! To make hooks api consistent with react-redux I need to also remove deps argument from hooks, too.

Quoted from reduxjs/react-redux#1272 (comment)

FWIW our conclusion internally has been that custom Hooks should avoid their own dependency argument, and people should just memoize if it's necessary. You have to learn that pattern anyway.

[prescottprue]

Let me know when it is ready for review, but no rush 😄 . Thanks again for all of the great work! 💯

[illuminist]

Yes, it's ready now.

I had verified with example projects in this repo. But somehow firestore and material example is broken with unrelated issues so I had to fixed them to work. I think it worth another PR though.

[prescottprue]

Nice work! Started reviewing this and everything is looking great so far 👏

I was thinking that it may be good to keep firebaseConnect and firestoreConnect as they are so they remain backwards compatible for folks that are not updated to 16.8 yet (at least in v3, that will probably change in future versions).

[prescottprue]

We can remove all of the gitbook dependencies - for some reason their cli automatically installs them