-
Notifications
You must be signed in to change notification settings - Fork 22
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
#9227 Refactor useTheme
to use useManagedStorageState
#9228
base: main
Are you sure you want to change the base?
Conversation
…b.com/pixiebrix/pixiebrix-extension into feature/9222_hide_sidebar_logo_policy merge origin
Just calling out that I'd like clarification on
|
useAsyncExternalStore(themeStorageSubscribe, themeStorage.get); | ||
|
||
const { data: managedStorageState, isLoading: isManagedStorageLoading } = | ||
useManagedStorageState(); |
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.
Let's use this as an opportunity to properly refactor useManagedStorageState
to return AsyncState
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.
This should probably use the following? I don't remember if there's a reason why we didn't use that other than that maybe the useAsyncExternalStore method didn't exist yet (I haven't checked) / or it just wasn't very valuable given that managed storage values should never change in practice:
useAsyncExternalStore
readManagedStorage
I think we then get error handling for free with those
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 was wondering the same thing, re why not use readManagedStorage + useAsyncExternalStore
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 was wondering the same thing, re why not use readManagedStorage + useAsyncExternalStore
I don't see a reason not to. The main difference will be it will be technically possible for the hook to return an error state. Whereas the current one could potentially stay isLoading
indefinitely. But I don't think the difference matters in practice
@@ -57,7 +59,7 @@ beforeEach(() => { | |||
useManagedStorageStateMock.mockReturnValue({ |
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.
NIT: use valueToAsyncState
from asyncStateUtils
jest.mocked(themeStorage.get).mockResolvedValue({ | ||
...customTheme, | ||
showSidebarLogo: themeValue, | ||
lastFetched: Date.now(), |
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.
NIT: save the time and use it to validate against:
jest.mocked(themeStorage.get).mockResolvedValue({ | |
...customTheme, | |
showSidebarLogo: themeValue, | |
lastFetched: Date.now(), | |
const lastFetched = Date.now(); | |
jest.mocked(themeStorage.get).mockResolvedValue({ | |
...customTheme, | |
showSidebarLogo: themeValue, | |
lastFetched, |
|
||
expect(result.current.activeTheme).toMatchObject({ | ||
...customTheme, | ||
lastFetched: expect.any(Number), |
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.
lastFetched: expect.any(Number), | |
lastFetched, |
# Conflicts: # src/hooks/useTheme.test.ts # src/hooks/useTheme.ts
Playwright test resultsDetails Open report ↗︎ Flaky testschrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I'd encourage us to declare the return type on exported functions
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -28,8 +28,14 @@ describe("useManagedStorageState", () => { | |||
it("waits on uninitialized state", async () => { | |||
const { result, waitFor } = renderHook(() => useManagedStorageState()); | |||
expect(result.current).toStrictEqual({ | |||
currentData: undefined, |
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.
Prefer loadingAsyncStateFactory here
@@ -55,10 +61,19 @@ describe("useManagedStorageState", () => { | |||
await waitForNextUpdate(); | |||
|
|||
expect(result.current).toStrictEqual({ | |||
currentData: { |
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.
Prefer valueToAsyncState here and override the refetch
prop matcher
What does this PR do?
Discussion
useManagedStorageState
hook throws on error, causing the unit test"uses activeTheme when error occurs in managed storage"
to fail. Do we want to add error-handling touseManagedStorageState
to preserve this behavior in the sidebar?