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.

Sign up for GitHub

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

Allow infinite dependent keys when using withOnyx #385

Merged
merged 4 commits into from
Oct 10, 2023
Merged

Allow infinite dependent keys when using withOnyx #385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64674d6
Re-implement dependent keys
tgolen Oct 4, 2023
822a982
Implement new tests
tgolen Oct 4, 2023
35be3c5
Ignore vscode files
tgolen Oct 4, 2023
e10755b
Improve comment accuracy
tgolen Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ dist/

# IDEs
.idea
.vscode

# Decrypted private key we do not want to commit
.github/OSBotify-private-key.asc
37 changes: 28 additions & 9 deletions lib/withOnyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ function getDisplayName(component) {
return component.displayName || component.name || 'Component';
}

/**
* Removes all the keys from state that are unrelated to the onyx data being mapped to the component.
*
* @param {Object} state of the component
* @param {Object} onyxToStateMapping the object holding all of the mapping configuration for the component
* @returns {Object}
*/
const getOnyxDataFromState = (state, onyxToStateMapping) => _.pick(state, _.keys(onyxToStateMapping));

export default function (mapOnyxToState, shouldDelayUpdates = false) {
// A list of keys that must be present in tempState before we can render the WrappedComponent
const requiredKeysForInit = _.chain(mapOnyxToState)
Expand Down Expand Up @@ -92,16 +101,20 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
this.checkEvictableKeys();
}

componentDidUpdate(prevProps) {
componentDidUpdate() {
// When the state is passed to the key functions with Str.result(), omit anything
// from state that was not part of the mapped keys.
const onyxDataFromState = getOnyxDataFromState(this.state, mapOnyxToState);

// If any of the mappings use data from the props, then when the props change, all the
// connections need to be reconnected with the new props
_.each(mapOnyxToState, (mapping, propertyName) => {
const previousKey = Str.result(mapping.key, prevProps);
const newKey = Str.result(mapping.key, this.props);
_.each(mapOnyxToState, (mapping, propName) => {
const previousKey = mapping.previousKey;
const newKey = Str.result(mapping.key, {...this.props, ...onyxDataFromState});
if (previousKey !== newKey) {
Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey);
delete this.activeConnectionIDs[previousKey];
this.connectMappingToOnyx(mapping, propertyName);
this.connectMappingToOnyx(mapping, propName);
}
});
this.checkEvictableKeys();
Expand All @@ -110,9 +123,8 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
componentWillUnmount() {
// Disconnect everything from Onyx
_.each(mapOnyxToState, (mapping) => {
const key = Str.result(mapping.key, this.props);
const connectionID = this.activeConnectionIDs[key];
Onyx.disconnect(connectionID, key);
const key = Str.result(mapping.key, {...this.props, ...getOnyxDataFromState(this.state, mapOnyxToState)});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one change that is different than the original PR. I realized that the unmount() was still trying to use the old method for getting the key name. This would probably lead to somewhat of a memory leak because it could cause Onyx to keep a bunch of connections that are never disconnected from when components unmount.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does removing this change introduce the same problem as previously?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can give it a try. That's a good hunch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it doesn't appear to. I think the changes for batched updates was probably the biggest help for preventing the performance problem we saw last time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... That's making me nervous.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just feel like this might cause more issues down the road.

Are you referring to the change in onUnmount() specifically, or this PR in general?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR in general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To dig into this further, I tried the following things:

  1. Reverted Onyx to the commit where the original PR was reverted. Then I copied these withOnyx changes to the code and tried it out. Result: I was not able to duplicate the original performance problem.
  2. Reverted Onyx to the commit of the original PR being merged, and tried it out in the app. Result: I experienced the following fatal error (caused by the initialValue change). After fixing only that fatal error I was still not able to reproduce the original performance problem, though I did see that the report switching was taking about 2-3x longer.

image
image

I'm not sure if this fatal error was what we experienced when the original PR was deployed. We definitely witnessed the app hanging and no one could report any JS console errors or record performance profiles, but that was about it.

Conclusion: I am feeling pretty good about the changes in this PR in general. The worst that can happen is we have to do another quick revert, which would stink, but it's also not the end of the world.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I did not try was using the original commit from Onyx and a commit from App on the same day... I can do that if you would like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. I'll continue my testing.

Onyx.disconnect(this.activeConnectionIDs[key], key);
});
}

Expand Down Expand Up @@ -244,7 +256,14 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) {
* component
*/
connectMappingToOnyx(mapping, statePropertyName) {
const key = Str.result(mapping.key, this.props);
const key = Str.result(mapping.key, {...this.props, ...getOnyxDataFromState(this.state, mapOnyxToState)});

// Remember the previous key so that if it ever changes, the component will reconnect to Onyx
// in componentDidUpdate
if (statePropertyName !== 'initialValue' && mapOnyxToState[statePropertyName]) {
// eslint-disable-next-line no-param-reassign
mapOnyxToState[statePropertyName].previousKey = key;
}
Comment on lines +263 to +266
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another small change from the previous PR. When initialValue was integrated, it lead to a situation where mapOnyxToState.initialValue: false, which caused mapOnyxToState[statePropertyName].previousKey to throw an error (because you cannot set .previousKey on the value false).


// eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs
this.activeConnectionIDs[key] = Onyx.connect({
Expand Down
86 changes: 65 additions & 21 deletions tests/unit/withOnyxTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@ import Onyx, {withOnyx} from '../../lib';
import ViewWithText from '../components/ViewWithText';
import ViewWithCollections from '../components/ViewWithCollections';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
import compose from '../../lib/compose';
import ViewWithObject from '../components/ViewWithObject';

const ONYX_KEYS = {
TEST_KEY: 'test',
COLLECTION: {
TEST_KEY: 'test_',
RELATED_KEY: 'related_',
STATIC: 'static_',
DEPENDS_ON_STATIC: 'dependsOnStatic_',
DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnStatic_',
DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC: 'dependsOnDependsOnDependsOnStatic_',
},
SIMPLE_KEY: 'simple',
SIMPLE_KEY_2: 'simple2',
Expand Down Expand Up @@ -215,10 +217,10 @@ describe('withOnyxTest', () => {
.then(() => {
rerender(<TestComponentWithOnyx collectionID="2" />);

// Note, when we change the prop, we need to wait for one tick for the
// component to update and one tick for batching.
return waitForPromisesToResolve().then(waitForPromisesToResolve);
// Note, when we change the prop, we need to wait for the next tick:
return waitForPromisesToResolve();
})
.then(waitForPromisesToResolve)
.then(() => {
expect(getByTestId('text-element').props.children).toEqual('test_2');
});
Expand All @@ -244,31 +246,73 @@ describe('withOnyxTest', () => {
});

it('should pass a prop from one connected component to another', () => {
const collectionItemID = 1;
const onRender = jest.fn();
const markReadyForHydration = jest.fn();
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.TEST_KEY, {test_1: {id: 1}});
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RELATED_KEY, {related_1: 'Test'});

// Given several collections with multiple items in each
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.STATIC, {
static_1: {name: 'Static 1', id: 1},
static_2: {name: 'Static 2', id: 2},
});

// And one collection will depend on data being loaded from the static collection
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_STATIC, {
dependsOnStatic_1: {name: 'dependsOnStatic 1', id: 3},
dependsOnStatic_2: {name: 'dependsOnStatic 2', id: 4},
});

// And one collection will depend on the data being loaded from the collection that depends on the static collection (multiple nested dependencies)
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_STATIC, {
dependsOnDependsOnStatic_3: {name: 'dependsOnDependsOnStatic 1', id: 5},
dependsOnDependsOnStatic_4: {name: 'dependsOnDependsOnStatic 2', id: 6},
});

// And another collection with one more layer of dependency just to prove it works
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.DEPENDS_ON_DEPENDS_ON_DEPENDS_ON_STATIC, {
dependsOnDependsOnDependsOnStatic_5: {name: 'dependsOnDependsOnDependsOnStatic 1'},
dependsOnDependsOnDependsOnStatic_6: {name: 'dependsOnDependsOnDependsOnStatic 2'},
});

// When a component is rendered using withOnyx and several nested dependencies on the keys
return waitForPromisesToResolve()
.then(() => {
const TestComponentWithOnyx = compose(
withOnyx({
testObject: {
key: `${ONYX_KEYS.COLLECTION.TEST_KEY}${collectionItemID}`,
},
}),
withOnyx({
testThing: {
key: ({testObject}) => `${ONYX_KEYS.COLLECTION.RELATED_KEY}${testObject.id}`,
},
}),
)(ViewWithCollections);
const TestComponentWithOnyx = withOnyx({
staticObject: {
key: `${ONYX_KEYS.COLLECTION.STATIC}1`,
},
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}`,
},
})(ViewWithCollections);
render(<TestComponentWithOnyx markReadyForHydration={markReadyForHydration} onRender={onRender} />);

// First promise is for the staticObject and dependentObject to load
return waitForPromisesToResolve();
})

// Second promise is for multiDependentObject to load
.then(waitForPromisesToResolve)

// Third promise is for extremeMultiDependentObject to load
.then(waitForPromisesToResolve)

// Then all of the data gets properly loaded into the component as expected with the nested dependencies resolved
.then(() => {
expect(onRender).toHaveBeenLastCalledWith({
collections: {}, markReadyForHydration, onRender, testObject: {id: 1}, testThing: 'Test',
markReadyForHydration,
onRender,
collections: {},
testObject: {isDefaultProp: true},
staticObject: {name: 'Static 1', id: 1},
dependentObject: {name: 'dependsOnStatic 1', id: 3},
multiDependentObject: {name: 'dependsOnDependsOnStatic 1', id: 5},
extremeMultiDependentObject: {name: 'dependsOnDependsOnDependsOnStatic 1'},
});
});
});
Expand Down