#9227 Refactor useTheme to use useManagedStorageState
#9228
Conversation
…b.com/pixiebrix/pixiebrix-extension into feature/9222_hide_sidebar_logo_policy merge origin
|
Just calling out that I'd like clarification on
|
# Conflicts: # src/hooks/useTheme.test.ts # src/hooks/useTheme.ts
Playwright test results
Details
Flaky testschrome › tests/modLifecycle.spec.ts › create, run, package, and update mod Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
| data, | ||
| isLoading: data == null, | ||
| }; | ||
| function useManagedStorageState() { |
There was a problem hiding this comment.
NIT: I'd encourage us to declare the return type on exported functions
There was a problem hiding this comment.
Note: Typescript infers the return type very well:
There was a problem hiding this comment.
This was from hovering over useManagedStorageState in useTheme. I would actually prefer to not require return types in cases like this
There was a problem hiding this comment.
Your call since it's a philosophical distinction. Explicitly declaring the type better decouples the protocol/interface from the implementation. (And is required to hide certain properties from the caller)
See rule for more context: https://typescript-eslint.io/rules/explicit-module-boundary-types/
</bike-shedding>
There was a problem hiding this comment.
That's a better rule than requiring return types on all functions (which can be a nightmare). I could accept this, but I don't feel strongly that we need it
There was a problem hiding this comment.
At this time I don't feel strongly about this either way. I would be open to experimenting with that rule
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9228 +/- ##
==========================================
+ Coverage 74.24% 75.10% +0.85%
==========================================
Files 1332 1369 +37
Lines 40817 42260 +1443
Branches 7634 7882 +248
==========================================
+ Hits 30306 31740 +1434
- Misses 10511 10520 +9 ☔ View full report in Codecov by Sentry. |
What does this PR do?
Discussion
useManagedStorageStatehook throws on error, causing the unit test"uses activeTheme when error occurs in managed storage"to fail. Do we want to add error-handling touseManagedStorageStateto preserve this behavior in the sidebar?