Add additional documentation for handling collections. Fix behavior of waitForCollectionCallback.#171
Conversation
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
LGTM left a small NAB. Thanks for fixing the bug and adding more docs on collections!
dangrous
left a comment
dangrous
left a comment
There was a problem hiding this comment.
Knowing very little about the context of this yet, this seems well written and understandable. Added a couple of places that could use clarification.
As for the actual content of it, I'll defer to someone who knows what they're talking about.
|
Updates look great to me! Resolves my questions perfectly. |
|
I was sort of holding off on reviewing this since @marcaaron and I were still having an ongoing discussion about whether a parameter is better or a separate method is better. Also, we should consider changing this param/method to be the default behavior for collections. @marcaaron Did you want to make some of those changes in this PR? |
|
We can keep discussing and I can make them in a follow up (or just not at all). This PR largely just improves documentation and fixes a bug on |
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`, report1); | ||
| ``` | ||
|
|
||
| or we can set many at once with `mergeCollection()`: |
There was a problem hiding this comment.
Can you please add a little more guidance here so people know which one to choose? I think adding information to say that when there are multiple keys, you should always use mergeCollect() because it optimizes all the UI updates. If it's a single key, it's fine to use either.
There was a problem hiding this comment.
Ah, I see you added all that information below. Maybe just add something here that says "See below for guidance on best practices"
| })(MyComponent); | ||
| ``` | ||
|
|
||
| This will return the initial collection as an object of collection member key/values. Changes to the individual member keys will modify the entire object and new props will be passed with each individual key update. |
There was a problem hiding this comment.
| This will return the initial collection as an object of collection member key/values. Changes to the individual member keys will modify the entire object and new props will be passed with each individual key update. | |
| This will add a prop to the component called `allReports` which is an object of collection member key/values. Changes to the individual member keys will modify the entire object and new props will be passed with each individual key update. The prop doesn't update on the initial rendering of the component until the entire collection has been read out of Onyx. |
|
|
||
| ```js | ||
| // Bad | ||
| _.each(reports, report => Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report)); // -> Connected component will update on each iteration |
There was a problem hiding this comment.
| _.each(reports, report => Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report)); // -> Connected component will update on each iteration | |
| _.each(reports, report => Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`, report)); // -> A component using withOnyx() will have it's state updated with each iteration |
| // Good | ||
| const values = {}; | ||
| _.each(reports, report => values[`${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`] = report); | ||
| Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, values); // -> Connected component will update just once |
There was a problem hiding this comment.
| Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, values); // -> Connected component will update just once | |
| Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, values); // -> A component using withOnyx() will only have it's state updated once |
|
Updated. Thanks for the notes @tgolen 🙇 |
4 participants
Details
Fixes issue found here: Expensify/App#10051 (comment)
Semi-related to: https://github.com/Expensify/Expensify/issues/222064
Automated Tests
I can add some but the ones that exist are passing.
Linked PRs
Expensify/App#10373