chore: switch to new context API #40
Conversation
|
Oh I guess I need some more coverage... |
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 6 6
Lines 88 90 +2
Branches 18 19 +1
=====================================
+ Hits 88 90 +2
Continue to review full report at Codecov.
|
|
I just checked... I think we will have to change the React peer dependency: - "react": ">=15"
+ "react": ">=16.3" What is the usual strategy for this? How to release such a change? |
|
@green-arrow What do you think of this pull request? |
| @@ -1,7 +1,8 @@ | |||
| { | |||
| "name": "react-firestore", | |||
| "version": "0.0.0-semantically-released", | |||
| "description": "React components to fetch data from firestore using render props", | |||
| "description": | |||
There was a problem hiding this comment.
@peternoordijk - Overall, it seems like not much changed in this file but the formatting got changed a decent amount. Do you have a formatter set up?
There was a problem hiding this comment.
Ah-- yes, I do. It's just VSCode with format on save. I guess I should've turned that off.
There was a problem hiding this comment.
@green-arrow just to be clear: do you want me to change this back? 馃槃
There was a problem hiding this comment.
@peternoordijk - Yeah, that would be awesome 馃槃 Thanks!
There was a problem hiding this comment.
@green-arrow no problem :) By the way I remember now what caused this change: yarn was doing all this formatting automatically 馃檮
There was a problem hiding this comment.
@green-arrow Hold on! This formatting is not done by yarn! This formatting happens in some git pre commit hook. Does that by any chance sound familiar to you? I'm very sure it happens in there because I was fixing the formatting using Vim this time, and just before committing it's changing right back.
There was a problem hiding this comment.
@peternoordijk - no worries then! Thanks for looking into it!
src/FirestoreProvider.js
Outdated
| render() { | ||
| return this.props.children; | ||
| return ( | ||
| <FirestoreContext.Provider value={{ firestoreDatabase, firestoreCache }}> |
There was a problem hiding this comment.
@peternoordijk - We should change this to value={this.state}, so that we don't cause a re-render of all consumers whenever this component updates.
Here is some clarification on this from the react docs: https://reactjs.org/docs/context.html#caveats
There was a problem hiding this comment.
Good point! I will make a commit later today with this change.
|
@peternoordijk - Thanks for this PR, and apologies for the delay. I just had a couple comments on it, but it looks great! As for your question around peer dependencies, we'll need to bump it in If you could take care of the review comments and bumping the peer dependency, I'll make sure to author the commit message correctly so a major version is released. Thanks again for all your work on this! |
|
@green-arrow no worries! I will later today post a new commit with the change. Thanks for looking into the PR! |
This PR switches this library so that it uses the new context API. Aside from that nothing really changes. However this switch required quite a lot of changes.
I'm not sure if we have to make a version change when releasing this code. I think this change will be breaking for React < 16.3 .
Why do we need this change?
Aside from the obvious technical debt I also got some code for React hooks coming in. The only problem with that is that those hooks depend on the
useContextfunction, which in turn needs this change.I got the hooks code working already, I just need some more unit tests. Once that's done you'll see another PR incoming 馃槃 (I just created a PR: it's #41 )