Skip to content
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 instructions and best practices to the README for dependent props #342

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

tgolen
Copy link
Collaborator

@tgolen tgolen commented Sep 14, 2023

@tgolen tgolen requested a review from a team as a code owner September 14, 2023 18:35
@tgolen tgolen self-assigned this Sep 14, 2023
@melvin-bot melvin-bot bot requested review from hayata-suenaga and removed request for a team September 14, 2023 18:35
mountiny
mountiny previously approved these changes Sep 15, 2023
@mountiny
Copy link
Contributor

Looks good to me, thank you!

hayata-suenaga
hayata-suenaga previously approved these changes Sep 15, 2023
README.md Outdated Show resolved Hide resolved
@parasharrajat
Copy link
Member

What I used was two WithOnyx subsriptions. like

export default withOnyx({
    report: {
        key: ({reportID) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
    }})
withOnyx({
    policy: {
        key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report.policyID}`,
    },
})(App);

Is this different from single WithOnyx anyhow?

Co-authored-by: Ana Margarida Silva <ams-amargarida@hotmail.com>
@tgolen tgolen dismissed stale reviews from hayata-suenaga and mountiny via 51956f7 September 15, 2023 18:11
@tgolen
Copy link
Collaborator Author

tgolen commented Sep 15, 2023

@parasharrajat It functionally results in the same end result, but with two withOnyx() you've now double-wrapped the component which is inefficient and doesn't allow batch rendering to work as well (it's just add more things to the rendering tree). The same thing can be done with a single withOnyx() and it works.

@parasharrajat
Copy link
Member

Glad to know. Thanks for the explanation.

@tgolen
Copy link
Collaborator Author

tgolen commented Sep 17, 2023

@mountiny @hayata-suenaga do one of you want to go ahead and merge this?

@hayata-suenaga hayata-suenaga merged commit 72aee68 into main Sep 18, 2023
3 checks passed
@hayata-suenaga hayata-suenaga deleted the tgolen-patch-2 branch September 18, 2023 00:02
@ospfranco ospfranco mentioned this pull request Sep 20, 2023
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants