Allow infinite dependent keys when using withOnyx by tgolen · Pull Request #355 · Expensify/react-native-onyx

tgolen · 2023-09-20T21:30:45Z

Details

I found that the dependent keys weren't being loaded because we were only passing props to the key functions. This won't contain the already loaded Onyx data because that data is only stored in the HOC's state.

The solution is to pass both the props and the state to the key functions.

There were a couple more fixes I had to do to ensure multiple levels of depencies are resolved as expected.

Related Issues

Fixes Expensify/App#27906

Automated Tests

All covered by unit tests

Linked PRs

TBD

tgolen · 2023-09-20T22:47:34Z

tests/unit/withOnyxTest.js

+                    },
+                })(ViewWithCollections);
                render(<TestComponentWithOnyx markReadyForHydration={markReadyForHydration} onRender={onRender} />);
+                return waitForPromisesToResolve();


I also found that the test wouldn't pass without this being added, which I think is normal. I'm not sure why it was removed as it existed when the test was originally written.

allroundexperts · 2023-09-20T22:48:32Z

tests/unit/withOnyxTest.js

+                const TestComponentWithOnyx = withOnyx({
+                    testObject: {
+                        key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`,
+                    },
+                    testThing: {
+                        key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`,
+                    },
+                })(ViewWithCollections);


Nice job updating these tests. Can you maybe add another key which is dependent on testThing? This will verify that n layer of nesting can be avoided by this flattened out structure!

Yeah, that's a great idea. I will add that!

allroundexperts · 2023-09-20T22:54:32Z

lib/withOnyx.js

+                    const previousKey = Str.result(mapping.key, {...prevProps, ...prevState});
+                    const newKey = Str.result(mapping.key, {...this.props, ...this.state});


Adding this.state directly might lead the exposure of some state properties like this.state.loading that shouldn't be visible to the end user. Do you think its a deal breaker?

It's probably not a deal-breaker (until someone starts abusing it), but it would be easy to also omit all keys from the state which aren't part of the mappings. I can add that.

That's perfect!

allroundexperts · 2023-09-21T00:25:22Z

lib/withOnyx.js

+                const mappingPropNames = _.keys(mapOnyxToState);
+                const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames);


Would be better to move this to another function since its being used at top as well.

allroundexperts · 2023-09-21T00:25:48Z

lib/withOnyx.js

-                const key = Str.result(mapping.key, {...this.props, ...this.state});
+                const mappingPropNames = _.keys(mapOnyxToState);
+                const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames);
+                const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData});


Probably, this can also go into that same function.

Sure, good idea. I've updated it to DRY up this logic and move it into a simple function defined at the top of the module.

allroundexperts · 2023-09-21T00:26:54Z

tests/unit/withOnyxTest.js

+                    staticObject: {
+                        key: `${ONYX_KEYS.COLLECTION.STATIC}1`,
                    },
-                    testThing: {
-                        key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${(testObject && testObject.id) || 0}`,
+                    dependentObject: {
+                        key: ({staticObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC}${(staticObject && staticObject.id) || 0}`,
+                    },
+                    multiDependentObject: {
+                        key: ({dependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC}${(dependentObject && dependentObject.id) || 0}`,
+                    },
+                    extremeMultiDependentObject: {
+                        key: ({multiDependentObject}) => `${ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC}${(multiDependentObject && multiDependentObject.id) || 0}`,


I'm lovin' it

allroundexperts

Super work!

jasperhuangg

nice stuff!

tgolen · 2023-09-21T08:06:23Z

Unfortunately, when tested in App, this caused major performance problems that locks up the browser. So, while it is a great change, I guess there are some other considerations that need to be made for this to be viable.

CC @chrispader @kidroca @hannojg I'd love to brainstorm with you guys on this one.

chrispader · 2023-10-09T14:55:50Z

Unfortunately, when tested in App, this caused major performance problems that locks up the browser. So, while it is a great change, I guess there are some other considerations that need to be made for this to be viable.

CC @chrispader @kidroca @hannojg I'd love to brainstorm with you guys on this one.

@tgolen sorry that i didn't respond yet. i'm gonna try to look into this issue this week, if nothing has changed 👍🏼

tgolen · 2023-10-09T16:22:17Z

Thanks, Chris! I redid the original PR in #385 and I am unable to reproduce any of the original performance problems, so I don't know that we need to do anything more yet at this point.

tgolen added 3 commits September 21, 2023 05:30

Testing dependent keys

bb607e3

Pass withOnyx state to key functions

e9c911a

Remove unused import

6c5a65d

tgolen commented Sep 20, 2023

View reviewed changes

allroundexperts reviewed Sep 20, 2023

View reviewed changes

tgolen added 2 commits September 21, 2023 07:10

Filter out unrelated data before passing state

0a06664

Add tests for nested dependencies and filter the state before passing

7865b47

allroundexperts reviewed Sep 21, 2023

View reviewed changes

DRY up method

ac7e983

allroundexperts approved these changes Sep 21, 2023

View reviewed changes

tgolen mentioned this pull request Sep 21, 2023

Dependent keys are not working as expected when using withOnyx Expensify/App#27906

Closed

6 tasks

tgolen changed the title ~~Testing dependent keys~~ [HOLD for App PR to be created] Allow infinite dependent keys when using withOnyx Sep 21, 2023

tgolen self-assigned this Sep 21, 2023

tgolen mentioned this pull request Sep 21, 2023

Add rules: Handle No Multiple Onyx in file Expensify/eslint-config-expensify#73

Merged

59 tasks

tgolen marked this pull request as ready for review September 21, 2023 00:56

tgolen requested a review from a team as a code owner September 21, 2023 00:56

melvin-bot bot requested review from jasperhuangg and removed request for a team September 21, 2023 00:56

tgolen changed the title ~~[HOLD for App PR to be created] Allow infinite dependent keys when using withOnyx~~ Allow infinite dependent keys when using withOnyx Sep 21, 2023

jasperhuangg approved these changes Sep 21, 2023

View reviewed changes

jasperhuangg merged commit 8031f94 into main Sep 21, 2023

tgolen deleted the tgolen-dependent-keys branch September 21, 2023 05:51

tgolen mentioned this pull request Sep 21, 2023

Revert "Allow infinite dependent keys when using withOnyx" #357

Merged

This was referenced Oct 4, 2023

[$500] [HOLD for payment 2023-11-21] Remove multiple withOnyx nestings Expensify/App#28824

Closed

Allow infinite dependent keys when using withOnyx #385

Merged

Julesssss mentioned this pull request Dec 11, 2023

[$1000] Web - Onyx: out-of-order updates Expensify/App#28737

Closed

6 tasks

		const previousKey = Str.result(mapping.key, {...prevProps, ...prevState});
		const newKey = Str.result(mapping.key, {...this.props, ...this.state});

		const mappingPropNames = _.keys(mapOnyxToState);
		const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames);

Conversation

tgolen commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Linked PRs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allroundexperts left a comment

Choose a reason for hiding this comment

Uh oh!

jasperhuangg left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen commented Sep 21, 2023

Uh oh!

chrispader commented Oct 9, 2023

Uh oh!

tgolen commented Oct 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgolen commented Sep 20, 2023 •

edited

Loading