Gracefully handle bad mergeCollection #272
Conversation
|
|
||
| throw new Error(`Provided collection doesn't have all its data belonging to the same parent. CollectionKey: ${collectionKey}, DataKey: ${dataKey}`); | ||
| hasCollectionKeyCheckFailed = true; | ||
| Logger.logAlert(`Provided collection doesn't have all its data belonging to the same parent. CollectionKey: ${collectionKey}, DataKey: ${dataKey}`); |
There was a problem hiding this comment.
We could keep throwing if you are on dev? In my opinion, this should throw. It means someone is not testing their API changes against the app. The fact that they are getting to staging or production is a problem. We are kind of making this more likely with this PR since an alert is easier to ignore than a throw.
There was a problem hiding this comment.
I feel like we should be fine with this for now, the typescript migration should start pretty soon and that should catch any of these malformed data updates.
Also just couple lines above when the data is "more" malformed, we just resolve the promise and log info which is same as here so thats what makes me think it should be fine
Logger.logInfo('mergeCollection() called with invalid or empty value. Skipping this update.');
There was a problem hiding this comment.
Yes, it's not a huge deal. But I am not following how the TS migration would prevent this. Updates from Web-Expensify get passed to Onyx.update() more or less blindly. It's gonna be a run time error that can't be prevented during compile time because the API could send the wrong type or even just a 🥔 emoji and Onyx would try to do something with it.
There was a problem hiding this comment.
To reiterate the main concern here: we are making these errors harder to catch on dev. We will find out about this as a staging deploy blocker, but it would be better to create no deploy blocker in the first place and catch it on dev.
There was a problem hiding this comment.
I agree that we can throw in dev. Created a PR #277
Gracefully handle bad mergeCollection updates so it doesn't block the merge queue
Details
Coming from https://expensify.slack.com/archives/C049HHMV9SM/p1689120594593919, bad merge updates can block Onyx's merge queue and cause issues in App. We should gracefully handle these.
Related Issues
Related to https://github.com/Expensify/Expensify/issues/290894
Automated Tests
N/A
Linked PRs