-
Notifications
You must be signed in to change notification settings - Fork 74
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
Allow infinite dependent keys when using withOnyx #355
Changes from 5 commits
bb607e3
e9c911a
6c5a65d
0a06664
7865b47
ac7e983
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ dist/ | |
|
||
# IDEs | ||
.idea | ||
.vscode | ||
|
||
# Decrypted private key we do not want to commit | ||
.github/OSBotify-private-key.asc |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,16 +92,21 @@ 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 mappingPropNames = _.keys(mapOnyxToState); | ||
const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); | ||
|
||
// 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, ...stateOnlyWithOnyxData}); | ||
if (previousKey !== newKey) { | ||
Onyx.disconnect(this.activeConnectionIDs[previousKey], previousKey); | ||
delete this.activeConnectionIDs[previousKey]; | ||
this.connectMappingToOnyx(mapping, propertyName); | ||
this.connectMappingToOnyx(mapping, propName); | ||
} | ||
}); | ||
this.checkEvictableKeys(); | ||
|
@@ -238,7 +243,14 @@ export default function (mapOnyxToState, shouldDelayUpdates = false) { | |
* component | ||
*/ | ||
connectMappingToOnyx(mapping, statePropertyName) { | ||
const key = Str.result(mapping.key, this.props); | ||
const mappingPropNames = _.keys(mapOnyxToState); | ||
const stateOnlyWithOnyxData = _.pick(this.state, mappingPropNames); | ||
const key = Str.result(mapping.key, {...this.props, ...stateOnlyWithOnyxData}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably, this can also go into that same function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// Remember the previous key so that if it ever changes, the component will reconnect to Onyx | ||
// in componentDidUpdate | ||
// eslint-disable-next-line no-param-reassign | ||
mapOnyxToState[statePropertyName].previousKey = key; | ||
|
||
// eslint-disable-next-line rulesdir/prefer-onyx-connect-in-libs | ||
this.activeConnectionIDs[key] = Onyx.connect({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
|
@@ -243,30 +245,65 @@ 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 three 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 depends 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} />); | ||
return waitForPromisesToResolve(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}) | ||
|
||
// 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'}, | ||
}); | ||
}); | ||
}); | ||
|
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.
Would be better to move this to another function since its being used at top as well.