-
Notifications
You must be signed in to change notification settings - Fork 84
Create onyx.update #133
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
Create onyx.update #133
Changes from all commits
b39b7b6
090fd9b
7e3661b
c73e3ef
7847316
668ef20
a52c944
8772195
1c97a1d
215f429
df22934
0e3f941
e3de274
971a825
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 | ||
|---|---|---|---|---|
|
|
@@ -780,6 +780,36 @@ function mergeCollection(collectionKey, collection) { | |||
| }); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Insert API responses and lifecycle data into Onyx | ||||
| * | ||||
| * @param {Array} data An array of objects with shape {onyxMethod: oneOf('set', 'merge'), key: string, value: *} | ||||
| */ | ||||
| function update(data) { | ||||
|
Member
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. FYI, there is a problem with this method's signature. It should return a Promise like other methods. When this is passed to Line 976 in 98260f1
decorateWithMetrics expects a promise as a return value for the function. Tackling here Expensify/App#10622. If we don't want to make this change, then the proposal is looking good on that issue. Also, metrics module method signatures are broken for the web as well. cc: @luacmartins
Contributor
Author
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. @parasharrajat thanks for investigating this. So can we just return the respective methods here? e.g.
Member
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. That won't work. There is a each loop here. Need to convert to map and return promises and handle those, etc. |
||||
| // First, validate the Onyx object is in the format we expect | ||||
| _.each(data, ({onyxMethod, key}) => { | ||||
| if (!_.contains(['set', 'merge'], onyxMethod)) { | ||||
| throw new Error(`Invalid onyxMethod ${onyxMethod} in Onyx update.`); | ||||
luacmartins marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| } | ||||
| if (!_.isString(key)) { | ||||
| throw new Error(`Invalid ${typeof key} key provided in Onyx update. Onyx key must be of type string.`); | ||||
|
Contributor
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. Can we give the index of the key for a more descriptive error message? We may end up with a big object and not know which key is the non-string, right?
Contributor
Author
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. That's a good idea. We could certainly add that as a follow up if we come across this issue! |
||||
| } | ||||
| }); | ||||
|
|
||||
| _.each(data, ({onyxMethod, key, value}) => { | ||||
| switch (onyxMethod) { | ||||
| case 'set': | ||||
| set(key, value); | ||||
| break; | ||||
| case 'merge': | ||||
| merge(key, value); | ||||
| break; | ||||
| default: | ||||
| break; | ||||
luacmartins marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| } | ||||
| }); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Initialize the store with actions and listening for storage events | ||||
| * | ||||
|
|
@@ -856,6 +886,7 @@ const Onyx = { | |||
| multiSet, | ||||
| merge, | ||||
| mergeCollection, | ||||
| update, | ||||
| clear, | ||||
| init, | ||||
| registerLogger, | ||||
|
|
@@ -883,6 +914,7 @@ function applyDecorators() { | |||
| mergeCollection = decorate.decorateWithMetrics(mergeCollection, 'Onyx:mergeCollection'); | ||||
| getAllKeys = decorate.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys'); | ||||
| initializeWithDefaultKeyStates = decorate.decorateWithMetrics(initializeWithDefaultKeyStates, 'Onyx:defaults'); | ||||
| update = decorate.decorateWithMetrics(update, 'Onyx:update'); | ||||
|
Contributor
Author
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 had missed adding the update method to the decorators for capturing metrics |
||||
| /* eslint-enable */ | ||||
|
|
||||
| // Re-expose decorated methods | ||||
|
|
@@ -891,6 +923,7 @@ function applyDecorators() { | |||
| Onyx.clear = clear; | ||||
| Onyx.merge = merge; | ||||
| Onyx.mergeCollection = mergeCollection; | ||||
| Onyx.update = update; | ||||
|
|
||||
| // Expose stats methods on Onyx | ||||
| Onyx.getMetrics = decorate.getMetrics; | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.