Skip to content
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

Closed
wants to merge 11 commits into from

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented Dec 15, 2024

Adds Zustand and persisted state management.

  • create 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.
  • Changes the file structue of the Router and context, so the Router can be used seperate from contexts (will allow for easier testing)
  • Adds a context to Router, to allows for redirects at path /, based on device name and persisted project id
  • properly redirects at path /. 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

return useStore(contextStore, selector)
}

return { Provider, useStoreHook, Context, nonPersistedStore }
Copy link
Contributor Author

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.

@gmaclennan
Copy link
Member

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:

  1. Changing how the app is composed so that providers can be over-ridden in tests.
  2. Deciding a pattern for how to add Zustand and persisted stores.
  3. Changing to using router context, and how initial navigation is resolved.

I think it's simplest to address these separately.

1. App provider composition & mocking for tests

The "app" is essentially what is created by <RouterProvider router={router} />. We need to wrap that in providers, and for integration tests some providers that rely on APIs specific to electron will not work, so they need to be mocked/replaced in tests. We can mock at different "layers":

  1. Keep the providers the same, and mock lower-level APIs e.g. mock localstorage, or mock queries to the backend.
  2. Have providers accept instances rather than create them, e.g. how QueryClientProvider currently works, and pass mocked singletons.
  3. Have providers create instances, but allow them to be overridden in tests.
  4. Expose an option on providers to create instances in a specific way, e.g. <StoreProvider persist={false} />.

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 Context.Provider) being used in tests to override the parent provider. There is lots of room for weird bugs from using this in the wrong way and ending up with mismatched instances of a Provider.

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 actions property in the store, that is "special" in that it can't be mutated, and instead keeping actions outside the actual zustand store, like in https://github.com/digidem/comapeo-mobile/blob/4e6d360a7b58ec1632eb7e191fc7a2907edf301c/src/frontend/hooks/draftObservation.ts#L1-L2. It avoids the possibility of mutating actions.

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.

@ErikSin
Copy link
Contributor Author

ErikSin commented Dec 18, 2024

replaced by #67

@ErikSin ErikSin closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants