-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: add zustand and persisted state management #64
Conversation
return useStore(contextStore, selector) | ||
} | ||
|
||
return { Provider, useStoreHook, Context, nonPersistedStore } |
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'm exposing the Context
and the Provider
here. The provider should be used by the app, the Context on the other hand can be used for testing to simulate the provider. This allows set up the same provider, without having to touch the persisted state, and injecting test values.
You can see an example of that here. The Value of persisted state determines where the user is redirected to. So I can easily simulate that state with a non persisted value by using the context and nonPersistedStore
There could be an argument that we should use persisted state in the tests, to better mimic the app. The problem is that we would be overwriting the actual persisted state, which might not be ideal for developer experience. We could use a different "key". Therefore testing environments would have its own persisted state, separate from the dev environment. This could get a little buggy as there could be things in persisted state that the test environment is using that we don't realize. We could make sure to clear persisted state at the end of every test file, but there would be some overhead with that.
@gmaclennan, @achou11, and @cimigree I'm curious if you have any insight or opinions of the best approach for testing persisted state.
There are a few big architectural decisions in here that are going to affect app development and testing going forwards. Sorry for a long response, but there are a lot of decisions made within this PR and it's worth being explicit about how we are solving these problems in ways that do not bite us in the future. There are 3 separate architectural changes in here:
I think it's simplest to address these separately. 1. App provider composition & mocking for tests The "app" is essentially what is created by
My preference would be (2) or (3) because for many tests we don't need to mock anything, and it gives us control over instances if we need to mutate them or read from them, however we should probably discourage tests that interact with implementation details (like instances used in providers) so (4) could be better if we want to force devs' hands, but I think that's too restrictive. What might this look like? // AppProvider.tsx
const queryClient = new QueryClient()
const storeA = createStoreA()
const storeB = createStoreB()
const comapeoClient = createComapeoClientConnectedToMainProcess()
export const AppProvider = ({ children }) => (
<QueryClientProvider client={queryClient>
<StoreAProvider store={storeA}>
<StoreBProvider store={storeB}>
<ComapeoApiProvider client={comapeoClient}>
{children}
//... // App.tsx
root.render(<RouterProvider router={router} wrap=AppProvider />) // test/utils/createTestProvider.ts
// opts could contain initial state, e.g. store state, or initial locale for intl
export function createTestProvider(opts) {
const queryClient = new QueryClient()
const storeA = createStoreAWithoutPersistence()
const storeB = createStoreBWithoutPersistence()
const comapeoClient = createComapeoClientInstanceWithTempStorage()
const AppProvider = ({ children }) => (
<QueryClientProvider client={queryClient}>
<StoreAProvider store={storeA}>
<StoreBProvider store={storeB}>
<ComapeoApiProvider client={comapeoClient}>
{children}
//...
return { Wrapper: AppProvider, queryClient, storeA, storeB, comapeoClient }
}
export function renderApp({ wrapper, ...rest }: RenderOptions) {
const router = createRouter()
return render(<RouterProvider router={router} wrapper={wrapper />, rest)
// or...
return render(<RouterProvider router=router />, { wrapper, ...rest }) // something.test.tsx
describe('something', () => {
const { Wrapper, comapeoClient } = createTestProvider({ storeAinitialState: { projectId: 'abcd' } })
await comapeoClient.setDeviceName('MyDevice')
renderApp({ wrapper: Wrapper })
}) I think this is going to enable the most flexibility, but generally we should avoid interacting with these singletons in tests - ideally any state would be created through simulating user interaction, however for the app initial state (setting device name and joining a project) then for convenience it may be worth skipping doing this as the user, and pre-populate the state. 2. Deciding a pattern for how to add Zustand and persisted stores. I think we could get in a muddle with the provider used in the app not being static (e.g. created by a function) and with the slightly different provider (from I think it's better to export a global singleton for Context and Provider, and allow the store instance to be passed in, e.g. import {useStore} from 'zustand';
const StoreAProvider = ({ children, store }) => {
return <StoreAContext.Provider value={store}>{children}</StoreAContext.Provider>
}
function useStoreA() => {
const store = useStore(StoreAContext)
return useStore(store)
} In terms of how we use Zustand, my personal preference is to avoid the way they recommend it in the docs of creating an 3. Changing to using router context, and how initial navigation is resolved. Maybe we can discuss this in a separate issue, to avoid this becoming too big? I think wrapping in context from a parent context is going to create complexity and edge-case bugs. We can discuss why and alternatives (I think what existed before was fine) in a separate issue. |
replaced by #67 |
Adds Zustand and persisted state management.
createPersistedStoreWithProvider
which provides a provider and hook for creating and consuming persisted state. Also provides a context and non-persisted store which can be used for testing./
, based on device name and persisted project id/
. Previously I was doing something a little hacky because it didnt feel like adding the state to context and injecting into the router was worth the complexity. But now there are 2 values needs for the redirect so it felt like creating the context and injecting into the router did make sense.this Pr and #63 breaks up #59 into a smaller chunks for reviewing