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

Jump to bottom

Add multiSet to update #305

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Add multiSet to update #305

merged 2 commits into from
Aug 23, 2023

Conversation

yuwenmemon
Copy link
Contributor

@yuwenmemon yuwenmemon commented Aug 18, 2023 •
edited
Loading

@marcaaron please review

Details

Adds multiset to onyx's update.

Related Issues

https://github.com/Expensify/Expensify/issues/309134

Automated Tests

Added 2 tests

Linked PRs

@yuwenmemon yuwenmemon force-pushed the yuwen-updateMultiSet branch 4 times, most recently from db902e3 to 0c35c72 Compare August 18, 2023 22:33
@yuwenmemon yuwenmemon self-assigned this Aug 18, 2023
@yuwenmemon yuwenmemon marked this pull request as ready for review August 18, 2023 22:37
@yuwenmemon yuwenmemon requested a review from a team as a code owner August 18, 2023 22:37
@melvin-bot melvin-bot bot requested review from youssef-lr and removed request for a team August 18, 2023 22:37
marcaaron
marcaaron requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Changes looks good. Maybe a good follow up eventually is to update the multiSet() logic so that it can group which of the keys belong to a collection and then update subscribers in a more optimized way.

react-native-onyx/lib/Onyx.js

Lines 985 to 992 in f96fa2d

_.each(data, (val, key) => {
// Update cache and optimistically inform subscribers on the next tick
cache.set(key, val);
notifySubscribersOnNextTick(key, val);
});
return Storage.multiSet(keyValuePairs)
.catch(error => evictStorageAndRetry(error, multiSet, data));

That might bite us in the future (sudden bad performance) if someone tries to use multiSet() to update a collection and also when there is a subscriber for the collection key.

lib/Onyx.js Outdated
}

throw new Error(`Invalid ${typeof key} key provided in Onyx multiSet. Onyx key must be of type string.`);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems not logically possible because all keys of an object must be strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be other types as well. At least, what I'm looking to do if prevent number-type keys at the very minimum. You can see this in my unit test.: https://github.com/Expensify/react-native-onyx/pull/305/files#diff-fb75d1d7a8480f8429b43b89f94665322ee4576f95220815a5f7f3275e99fb40R605

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, not trying to split hairs. I thought they would be cast to string. At least when iterating over them with Object.keys(), but maybe numbers are unique 🤷‍♂️

2023-08-21_13-06-47

NAB, I am not sure why should we not use a number as a key for Onyx. We would never do it. But doesn't seem like it should be explicitly prevented for any reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice, okay. I can remove those checks then.

tests/unit/onyxTest.js
];

try {
// When we pass it to Onyx.update
Onyx.update(data);
} catch (error) {
// Then we should expect the error message below
expect(error.message).toEqual('Invalid onyxMethod multiSet in Onyx update.');
expect(error.message).toEqual('Invalid onyxMethod murge in Onyx update.');
Copy link
Contributor

Choose a reason for hiding this comment

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

murge?! 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha - yeah open to ideas for better names for a "wrong" onyxMethod.

@yuwenmemon
Copy link
Contributor Author

Maybe a good follow up eventually is to update the multiSet() logic so that it can group which of the keys belong to a collection and then update subscribers in a more optimized way.

Yeah I thought about that. Seems like a good issue for a follow-up since there isn't an immediate need for it.

@marcaaron marcaaron self-requested a review August 22, 2023 20:59
@marcaaron
Copy link
Contributor

LGTM

@yuwenmemon
Copy link
Contributor Author

Follow-up issue is here: #313

@yuwenmemon yuwenmemon merged commit b653c5c into main Aug 23, 2023
3 checks passed
@yuwenmemon yuwenmemon deleted the yuwen-updateMultiSet branch August 29, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcaaron marcaaron marcaaron approved these changes

@youssef-lr youssef-lr Awaiting requested review from youssef-lr

Assignees

@yuwenmemon yuwenmemon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yuwenmemon @marcaaron