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: creates persisted store for Active Project Id using Zustand #67

Merged
merged 13 commits into from
Dec 19, 2024
33 changes: 32 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@
"typescript-eslint": "^8.15.0",
"vite": "^5.4.11",
"vite-plugin-svgr": "^4.3.0",
"vitest": "2.1.8"
"vitest": "2.1.8",
"zustand": "5.0.2"
},
"overrides": {
"better-sqlite3": "11.5.0"
Expand Down
41 changes: 41 additions & 0 deletions src/renderer/src/App.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { CssBaseline, ThemeProvider } from '@mui/material'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import { RouterProvider, createRouter } from '@tanstack/react-router'

import { theme } from './Theme'
import { ApiProvider } from './contexts/ApiContext'
import { IntlProvider } from './contexts/IntlContext'
import {
PersistedActiveProjectIdProvider,
createActiveProjectIdStore,
} from './contexts/persistedState/PersistedProjectId'
import { routeTree } from './routeTree.gen'

const queryClient = new QueryClient()

const router = createRouter({ routeTree })

declare module '@tanstack/react-router' {
interface Register {
router: typeof router
}
}

const PersistedProjectIdStore = createActiveProjectIdStore({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: we haven't discussed this, but generally we've followed the pattern of naming instances with lowercase, and factory functions / classes with capitalized names, I think it's good to continue that pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPersisted: true,
})

export const App = () => (
<ThemeProvider theme={theme}>
<CssBaseline />
<IntlProvider>
<QueryClientProvider client={queryClient}>
<PersistedActiveProjectIdProvider store={PersistedProjectIdStore}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: this is really <ActiveProjectIdProvider> - the provider is the same whether it is persisted or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<ApiProvider>
<RouterProvider router={router} />
</ApiProvider>
</PersistedActiveProjectIdProvider>
</QueryClientProvider>
</IntlProvider>
</ThemeProvider>
)
21 changes: 21 additions & 0 deletions src/renderer/src/contexts/persistedState/PersistedProjectId.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { createPersistedStoreWithProvider } from './createPersistedState'

type PersistedActiveProjectId = { projectId: string }

const { Provider, createStore, useCurrentStore, useActions } =
createPersistedStoreWithProvider<PersistedActiveProjectId>({
slice: { projectId: 'newId' },
actions: {
setProjectId: (newProjectId: string) => (set) => {
set({ projectId: newProjectId })
},
},
persistedStoreKey: 'ActiveProjectId',
})

export {
Provider as PersistedActiveProjectIdProvider,
createStore as createActiveProjectIdStore,
useCurrentStore as usePersistedActiveProjectIdStore,
useActions as usePersistedActiveProjectIdActions,
}
128 changes: 128 additions & 0 deletions src/renderer/src/contexts/persistedState/createPersistedState.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
import { createContext, useContext, type ReactNode } from 'react'
import {
createStore as createZustandStore,
useStore,
type StateCreator,
type StoreApi,
} from 'zustand'
import { persist } from 'zustand/middleware'

type PersistedStoreKey = 'ActiveProjectId'

/**
* Follows the pattern of injecting persisted state with a context. See
* https://tkdodo.eu/blog/zustand-and-react-context. Allows for easier testing
*/
export function createPersistedStoreWithProvider<T>({
slice,
actions,
persistedStoreKey,
}: {
slice: T
actions: StoreActions<T>
persistedStoreKey: PersistedStoreKey
}) {
const Context = createContext<ReturnType<typeof createStore<T>> | null>(null)

const Provider = ({
children,
store,
}: {
children: ReactNode
store: ReturnType<typeof createStore<T>>
}) => {
return <Context.Provider value={store}>{children}</Context.Provider>
}

function useCurrentContext() {
const context = useContext(Context)
if (!context) {
throw new Error(`${persistedStoreKey} context not properly initialized`)
}
return context
}

// Hook to select store state
function useCurrentStore<Selected>(selector: (state: T) => Selected) {
const context = useCurrentContext()
return useStore(context.store as StoreApi<T>, selector)
}

function useActions() {
const context = useCurrentContext()
return context.actions
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im having trouble with this typing. Im not able to get the actions back, any chance you see the problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd need more time to try to figure it out. My gut feeling is that attempting to create a helper function for creating a store and provider is a "premature abstraction", by which I mean an abstraction made before being clear about how something is going to be re-used, and being clear about the goal of the abstraction. I assume the goal is to avoid repeating code for each store, but the costs are:

  • More complicated code and typings
  • The need to change things in many places when you want to add additional functionality to a store, like configuring migrations
  • More code to test and maintain

I'm not sure it really saves that much code or makes it easier to understand what is happening vs. just following the pattern with some repeated code each time. Doing it without the helper will certainly make the types easier. I imagine the issue is related to the wrapping of actions and generics, but it would take me an hour or so to figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that there is a lot of boiler plate code and we want all our stores to look the same. Also I am anticipating that we are going to have a few of these stores as In mobile we have quite a few persisted store. I think there will be slightly less for desktop (we don't need a persisted observation store for example). But I think there are still going to be a few stores.

We are expecting all of them to return:

  1. a provider, that takes the value as props
  2. a createStore function that returns a persisted or non persisted store
  3. a useStore hook (with atomic selectors)
  4. a useStoreAction hook. (we also do not want the actions to be in the zustand store).

We have a lot of very specific requirements for these stores. So it feels like there is alot of room for error by not abstracting this?

Perhaps it would be best to abstract some of these things, but not all of them? Or do you think abstraction is not necessary at all?

Copy link
Member

@gmaclennan gmaclennan Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to know exactly what makes sense to abstract until you have started writing some actual stores. I think it's generally fine to start with repeated boilerplate. The challenge with writing the abstractions first is that you can end up trapping yourself in a corner with abstractions that don't actually make sense or make the code harder to write, or you find that every change requires changing lots of pieces of the abstraction. This is very similar to the guidelines we have talked about with React components - it's best to abstract small re-usable parts of the code, rather than creating a large abstraction (component) that tries to manage lots of different things, and ends up adding to the maintenance burden.

An example here might be a future need to have a Zustand store that is not / should not be persisted, and then having to adjust the abstractions so that they can handle that case.

Looking at this code, I think the most "boiler-platey" part which would be repeated and is unlikely to change is the hooks, so you could create an abstraction like:

function createHooks<TStoreState, TStoreActions>(
  context: React.Context<{
    store: StoreApi<TStoreState>;
    actions: TStoreActions;
  }>,
) {
  function useContextValue() {
    const value = useContext(context);
    if (!value) {
      throw new Error('Must set up the provider first');
    }
    return value;
  }

  function useStoreState(): TStoreState;
  function useStoreState<S>(selector: (state: TStoreState) => S): S;
  function useStoreState<S>(selector?: (state: TStoreState) => S) {
    const {store} = useContextValue();
    return useStore(store, selector!);
  }

  function useStoreActions() {
    const {actions} = useContextValue();
    return actions;
  }

  return {useStoreState, useStoreActions};
}

That feels like something that is unlikely to change and is fairly simple to understand and test (if tests are needed), and is limited in scope about what it is trying to do (takes a single argument and turns it into two hooks)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


return {
Provider,
createStore: ({ isPersisted }: { isPersisted: boolean }) => {
return isPersisted
? createStore({ isPersisted: true, persistedStoreKey, actions, slice })
: createStore({ actions, slice, isPersisted: false })
},
useCurrentStore,
useActions,
}
}

function createPersistedStore<T>(
...args: Parameters<typeof createPersistMiddleware<T>>
) {
const store = createZustandStore<T>()(createPersistMiddleware(...args))
store.setState((state) => ({
...state,
...args[0],
}))

return store
}

function createPersistMiddleware<State>(
slice: StateCreator<State>,
persistedStoreKey: PersistedStoreKey,
) {
return persist(slice, {
name: persistedStoreKey,
})
}

type ActionCreator<T> = (
// eslint-disable-next-line @typescript-eslint/no-explicit-any
newState: any,
) => (set: StoreApi<T>['setState'], get: StoreApi<T>['getState']) => void

type StoreActions<T> = { [key: string]: ActionCreator<T> }

type createStoreProps<T> = {
slice: T
actions: StoreActions<T>
} & (
| { isPersisted: false }
| { isPersisted: true; persistedStoreKey: PersistedStoreKey }
)

export function createStore<T>(props: createStoreProps<T>) {
let store: StoreApi<T>

if (!props.isPersisted) {
store = createZustandStore<T>()(() => ({ ...props.slice }))
} else {
store = createPersistedStore<T>(
() => ({ ...props.slice }),
props.persistedStoreKey,
)
}

const actions = Object.fromEntries(
Object.entries(props.actions).map(([key, action]) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const wrappedAction = (newState: any) => {
return action(newState)(store.setState, store.getState) // Pass `setState` and `getState`
}

return [key, wrappedAction]
}),
) as StoreActions<T>

return { store, actions }
}
13 changes: 2 additions & 11 deletions src/renderer/src/index.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,9 @@
import { RouterProvider, createRouter } from '@tanstack/react-router'
import { createRoot } from 'react-dom/client'

import { routeTree } from './routeTree.gen'

import './index.css'

const router = createRouter({ routeTree })

declare module '@tanstack/react-router' {
interface Register {
router: typeof router
}
}
import { App } from './App'

const root = createRoot(document.getElementById('app') as HTMLElement)

root.render(<RouterProvider router={router} />)
root.render(<App />)
6 changes: 6 additions & 0 deletions src/renderer/src/routes/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { useLayoutEffect } from 'react'
import { useOwnDeviceInfo } from '@comapeo/core-react'
import { createFileRoute, useNavigate } from '@tanstack/react-router'

import { usePersistedActiveProjectIdStore } from '../contexts/persistedState/PersistedProjectId'

export const Route = createFileRoute('/')({
component: RouteComponent,
})
Expand All @@ -13,10 +15,14 @@ function RouteComponent() {
const navigate = useNavigate()
const { data } = useOwnDeviceInfo()
const hasCreatedDeviceName = data?.name !== undefined
const projectId = usePersistedActiveProjectIdStore((store) => store.projectId)

useLayoutEffect(() => {
if (!hasCreatedDeviceName) {
navigate({ to: '/Onboarding' })
}
if (!projectId) {
navigate({ to: '/Onboarding/CreateJoinProjectScreen' })
} else {
navigate({ to: '/tab1' })
}
Expand Down
Loading