-
Notifications
You must be signed in to change notification settings - Fork 89
WIP fix: Make storage calls async and await the init task #727
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
base: main
Are you sure you want to change the base?
WIP fix: Make storage calls async and await the init task #727
Conversation
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.
I think we still need to return promises in all these cases. There is still code that uses them, isn't there?
5cf97ad to
e176ab0
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e176ab0db3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function update<TKey extends OnyxKey>(data: Array<OnyxUpdate<TKey>>): Promise<void> { | ||
| // First, validate the Onyx object is in the format we expect | ||
| for (const {onyxMethod, key, value} of data) { | ||
| if (!Object.values(OnyxUtils.METHOD).includes(onyxMethod)) { | ||
| throw new Error(`Invalid onyxMethod ${onyxMethod} in Onyx update.`); | ||
| } | ||
| if (onyxMethod === OnyxUtils.METHOD.MULTI_SET) { | ||
| // For multiset, we just expect the value to be an object | ||
| if (typeof value !== 'object' || Array.isArray(value) || typeof value === 'function') { | ||
| throw new Error('Invalid value provided in Onyx multiSet. Onyx multiSet value must be of type object.'); | ||
| return OnyxUtils.getDeferredInitTask().promise.then(() => { | ||
| // First, validate the Onyx object is in the format we expect | ||
| for (const {onyxMethod, key, value} of data) { | ||
| if (!Object.values(OnyxUtils.METHOD).includes(onyxMethod)) { |
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.
Preserve synchronous validation errors in update
Wrapping the entire body in getDeferredInitTask().promise.then(...) makes the validation errors throw asynchronously. Callers (and tests like tests/unit/onyxTest.ts around the “should throw an error…” case) currently rely on a synchronous throw from Onyx.update to fail fast when invalid data is passed; with this change, the same misuse becomes an unhandled promise rejection unless every caller awaits/catches the returned promise. This is a behavior change that breaks existing try/catch usage and can let invalid updates slip past immediate error handling.
Useful? React with 👍 / 👎.
@roryabraham @sosek108 @tgolen
Details
This PR makes Onyx storage calls async so we can await the initialization task before executing them.
Related Issues
GH_LINK
Automated Tests
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop