Update Listener When Props Change #10
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 5 6 +1
Lines 58 76 +18
Branches 9 14 +5
=====================================
+ Hits 58 76 +18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
First off, thank you for taking the time to tackle this problem!
I like the direction of this and would definitely like to see it applied to collection as well.
My one thought on the approach overall would be do we actually want to fully reset to the default state when props change, or do we just want to flip the isLoading flag? Completely resetting means the caller no longer has access to data, so performing any kind of transition directly between states becomes impossible. The caller would have to first transition to an empty loading state until the new data comes in. If we keep data unchanged, that becomes a choice for the caller instead of a necessity.
|
@green-arrow That's a good call. I modified If you're good with this, I'll take a look at doing the same thing for collections. |
|
@damonbauer - this looks good. Feel free to go ahead and add this to collection. |
|
@green-arrow I'm not super sure how you'd prefer tests to be set up/organized, but I added them. I tried to reduce some of the boilerplate for each of the different prop tests in Note: I added |
|
@green-arrow Have you had a chance to review this? |
There was a problem hiding this comment.
@damonbauer - Sorry this took so long for me to get to, I've been pretty busy at work and home for the past couple weeks.
Just a couple comments around removing lodash. Everything looks great though!
package.json
Outdated
| @@ -53,6 +53,7 @@ | |||
| "enzyme": "^3.3.0", | |||
| "enzyme-adapter-react-16": "^1.1.1", | |||
| "kcd-scripts": "^0.32.1", | |||
| "lodash.isequal": "^4.5.0", | |||
There was a problem hiding this comment.
@damonbauer - I'd prefer to keep the dependencies on this project to a minimum. Since this is only used in one spot, let's just replace it and remove the dependency.
src/FirestoreCollection.js
Outdated
| } | ||
|
|
||
| componentWillReceiveProps(nextProps) { | ||
| if (!isEqual(nextProps, this.props)) { |
There was a problem hiding this comment.
@damonbauer - Since we're already testing for each individual prop in the tests, we can just do the same here and remove the dependency on lodash.
|
Alright, @green-arrow, I've removed the lodash dependency. I added a small "deep equal" utility that I'm using to compare the |
I ran into #9 and thought I might be able to fix the issue.
I added a
componentWillReceivePropslifecycle method that compares incoming & existing props. If thepathprop is different, it's safe to assume we want to fetch new data. So, I unsubscribed the active listener, set state back to the "default state", and set up a new listener for the new path.@green-arrow @mikekellyio - if this direction makes sense, I can make the same change to
FirestoreCollection; I just wanted to make sure this concept made sense before I spent more time on it.