[Theme] Handle async errors #4599
Conversation
|
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1947 tests passing in 876 suites. Report generated by 🧪jest coverage report action from 8e8fef2 |
frandiox
requested review from
karreiro,
jamesmengo and
a team
and removed request for
karreiro
This comment has been minimized.
This comment has been minimized.
| }) | ||
| }) | ||
| .catch(abort) |
There was a problem hiding this comment.
Instead of .catch()ing on each of these could we wrap the whole thing in a try/catch? Maybe not with the promises?
There was a problem hiding this comment.
It would be much more verbose with async promises than .catch, though. We could also pass it as the second param in .then(() => {}, abort) but I think most people are more familiar with .catch 🤔
| const [result] = await bulkUploadThemeAssets(Number(themeId), [{key: fileKey, value: content}], adminSession) | ||
| const [result] = await bulkUploadThemeAssets(Number(themeId), [{key: fileKey, value: content}], adminSession) | ||
|
|
||
| if (!result?.success) { |
There was a problem hiding this comment.
(nit) Would prefer result.errors here
There was a problem hiding this comment.
I'm not familiar with the underlying API this calls in cli-kit, so not sure if something can have a "warning" in errors or something like that. It looks like success is just response.status === 200, and errors are at least {} instead of undefined.
cc @jamesmengo thoughts?
| Promise.all([ | ||
| themeCreationPromise, | ||
| uploadJobPromise.then((result) => result.promise), | ||
| deleteJobPromise.then((result) => result.promise), |
There was a problem hiding this comment.
I think this would have already run on L51? I might be misreading this.
There was a problem hiding this comment.
Afaik, the .then creates a new Promise, so in line 51 we create "promise A" that resolves to X, and here we create "promise B" that also resolves to X. But they are still different promises and, since "promise A" is conditional, we can't use it to add the catch (because we might be adding it to something else than the delete promise).
I think? 🤔
| : deleteJobPromise.then((result) => result.promise) | ||
|
|
||
| if (options?.backgroundWorkCatch) { | ||
| // Aggregate all backgorund work in a single promise and handle errors |
There was a problem hiding this comment.
| // Aggregate all backgorund work in a single promise and handle errors | |
| // Aggregate all background work in a single promise and handle errors |
|
@shopify/app-inner-loop, Could you please take a look at this PR? It seems like the merge is blocked by that requirement. |
WHY are these changes introduced?
Related to #4576 and #4598
A proper request retry + token refresh is still needed but this PR tries to handle async errors more gracefully.
WHAT is this pull request doing?
Ensure we print async errors properly with a red banner, and avoid process exit in some situations:
For example, when you lose your internet connection while polling:
During the initial sync, if there are too many delete errors or a single upload error:
How to test your changes?
Create fake errors around the modified code or directly disconnect your internet + reconnect it within 15 seconds.
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist