-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add additional documentation for handling collections. Fix behavior of waitForCollectionCallback
.
#171
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM left a small NAB. Thanks for fixing the bug and adding more docs on collections!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
README.md
Outdated
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${report1.reportID}`, report1); | ||
``` | ||
|
||
or we can set many at once with `mergeCollection()`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see you added all that information below. Maybe just add something here that says "See below for guidance on best practices"
README.md
Outdated
})(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
README.md
Outdated
|
||
```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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_.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 |
README.md
Outdated
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙇 |
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