cli icon indicating copy to clipboard operation
cli copied to clipboard

[Theme] Handle async errors

Open frandiox opened this issue 1 year ago β€’ 2 comments

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:

  • On file delete / upload error in the remote theme, we show the error but the process continues. This is because the local development can continue working with local files in most cases, and is up to the developer to halt the process after seeing the error.
  • On polling errors, wait for 5 errors before exiting the process. This is because it might recover from one or two failed attempts since it still compares local checksums with latest.

For example, when you lose your internet connection:

image

In the above, should we show only the first error in terminal instead of showing it 5 times?

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:

  • [ ] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • [ ] Existing analytics will cater for this addition
  • [ ] PR includes analytics changes to measure impact

Checklist

  • [ ] I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • [ ] I've considered possible documentation changes

frandiox avatar Oct 08 '24 07:10 frandiox

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/advanced-edits
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/app-inner-loop

github-actions[bot] avatar Oct 08 '24 07:10 github-actions[bot]

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
72.73% (+0.09% πŸ”Ό)
8563/11774
🟑 Branches
69.72% (+0.11% πŸ”Ό)
4205/6031
🟑 Functions
71.69% (-0.03% πŸ”»)
2211/3084
🟑 Lines
73.06% (+0.09% πŸ”Ό)
8105/11094
Show new covered files 🐣
St.:grey_question:
File Statements Branches Functions Lines
🟒
... / function-upload-url-generate.ts
100% 100% 100% 100%
Show files with reduced coverage πŸ”»
St.:grey_question:
File Statements Branches Functions Lines
🟒
... / app.test-data.ts
91.4% (-0.54% πŸ”»)
91.09% (-0.09% πŸ”»)
81.01% (-1.27% πŸ”»)
90.8% (-0.57% πŸ”»)
🟒
... / app.ts
87.07%
70.89% (-0.72% πŸ”»)
92% 88.37%
🟒
... / specification.ts
93.1% (-1.81% πŸ”»)
90.48%
87.5% (-0.5% πŸ”»)
92% (-2.12% πŸ”»)
🟒
... / function.ts
86.36% (-0.59% πŸ”»)
86.36% 83.33%
86.36% (-0.59% πŸ”»)
πŸ”΄
... / extension.ts
55.26% (-0.29% πŸ”»)
50% 57.14%
56.76% (-0.06% πŸ”»)
🟑
... / update-extension.ts
64.86% (-5.14% πŸ”»)
54.55% (-3.79% πŸ”»)
60%
68.75% (-5.54% πŸ”»)
🟑
... / build.ts
74.49%
59.09% (-2.27% πŸ”»)
75.76% 72.22%
🟒
... / extension.ts
91.4% (+0.09% πŸ”Ό)
73.58%
91.3% (-0.36% πŸ”»)
91.21% (+0.1% πŸ”Ό)
πŸ”΄
... / app-management-client.ts
20.75% (-0.09% πŸ”»)
10.26%
22.58% (-0.25% πŸ”»)
19% (-0.09% πŸ”»)
πŸ”΄
... / partners-client.ts
26.87% (-0.2% πŸ”»)
40%
18.18% (-0.34% πŸ”»)
26.56% (-0.21% πŸ”»)
🟑
... / fs.ts
62.5%
84.62% (-7.05% πŸ”»)
58.97% 62.5%

Test suite run success

1947 tests passing in 876 suites.

Report generated by πŸ§ͺjest coverage report action from 8e8fef2eebce91544f9f63db27c1e4dadcaf425d

github-actions[bot] avatar Oct 08 '24 08:10 github-actions[bot]

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

github-actions[bot] avatar Oct 09 '24 06:10 github-actions[bot]

@shopify/app-inner-loop, Could you please take a look at this PR? It seems like the merge is blocked by that requirement.

karreiro avatar Oct 16 '24 07:10 karreiro