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

add vike-react-zustand #57

Open
wants to merge 114 commits into
base: main
Choose a base branch
from
Open

add vike-react-zustand #57

wants to merge 114 commits into from

Conversation

nitedani
Copy link
Member

@nitedani nitedani commented Dec 26, 2023

What do you think about this? Abstracting the creation of the context for the store, is tricky.
The current implementation doesn't support multiple stores, but I think it's already useful. For multiple stores, support can be added using a unique key passed to createUseStore, then the key can be used as an identifier to get the right store from the context.

Or a completely different implementation, using + files.

@nitedani nitedani marked this pull request as ready for review December 26, 2023 19:21
@brillout
Copy link
Member

What do you think about this?

Yes, that's definitely valuable 💯

tricky

Yes, in particular sending the initial store state set by SSR to the client-side. Speaking of which, this part doesn't work yet right?

The current implementation doesn't support multiple stores, but I think it's already useful.

👍

For multiple stores, support can be added using a unique key passed to createUseStore, then the key can be used as an identifier to get the right store from the context.

Yes, I think a key is always required in the context of SSR. Just like it's required for vike-react-query.

Or a completely different implementation, using + files.

I don't know yet 👀 maybe we can mull over this once we've got the basic functionalities working (i.e. the state hydration thingy).

@nitedani
Copy link
Member Author

@brillout

the state hydration thingy

added

@nitedani
Copy link
Member Author

Should we pass everything to the client on initial navigation, or only the return value of server?
https://github.com/vikejs/vike-react/pull/57/files#diff-27a888a36f2dca710aeb519e21c30e5dad4ae192110374c14481703b23a7f2afR22-R24

@brillout
Copy link
Member

I don't think we have a choice? AFAICT the entire state used for hydration should be exactly the same than the state used for SSR.

@nitedani
Copy link
Member Author

Ok. I think it's ready. After using typeof window === "undefined" multiple times, I had an idea: maybe the environment can be set on the pageContext by Vike: pageContext.env: 'client' | 'server' and then we could use that instead.

@brillout
Copy link
Member

Ok. I think it's ready. After using typeof window === "undefined" multiple times, I had an idea: maybe the environment can be set on the pageContext by Vike: pageContext.env: 'client' | 'server' and then we could use that instead.

👍 Good idea. How about we name it pageContext._env so that end users are incentivized to use import.meta.env.SSR instead so that they get code splitting.

I was also thinking that, for vike-react-query, Vike could set import.meta.env.DEV.

@brillout
Copy link
Member

brillout commented Jan 2, 2024

I think the following proxy is misleading:

  new Proxy(useStore, {
    get(target, p: keyof ReturnType<typeof createZustand>) {
      return target()[p]
    },
    apply(target, _this, [selector]) {
      return target()(selector)
    }
  })

Because it only works when respecting React's hook rules. And it doesn't really add any value, so how about we remove that proxy?

Instead, I think what we could do is this:

// Only works on the client-side
import { getStore } from 'vike-react-zustand/client'

// `store` is the store object provided by Zustand
const store = getStore()

@nitedani
Copy link
Member Author

nitedani commented Jan 2, 2024

I agree, it's misleading because it's bound to the context. The issue is not solved with your idea though, getStore can still be called outside of the context. The function name should start with use to signal to the user that it should be used in a component. I think the current proxy has that advantage.

RIght now you can do:

function Component(){
	useStore.getState()
}

Can't do:

function Component(){
}
useStore.getState()

Can't do:

function Component(){
	if(something)
		useStore.getState()
}

But can't do neither:

function Component(){
}
getStore()

Maybe, we could export a function useStoreApi or something?
Or, we could remove the functionality, and don't let users use the store api at all?
Or change the throw new Error("Store has no provider") to a usage warning message?
Or, I don't know about this, but maybe there is a way to use the context without respecting the rule of hooks?

Either way, I agree we should remove the proxy for now, and maybe find a solution later to expose the store api.

@nitedani
Copy link
Member Author

nitedani commented Jan 2, 2024

What do you think about the change? I added useStoreApi and removed the Proxy. The type of create is still misleading on userland though, because we 1:1 use the zustand type. I also don't know yet how we can cleanly make useStoreApi compatible with multiple stores in the future. Maybe require users to call it like: useStoreApi(useStore) and we can set useStore.__storeKey in the library.

@nitedani
Copy link
Member Author

nitedani commented Jan 2, 2024

Started adding support for multiple stores, but I'll leave it like this for now. Just wanted to know if it will be possible to do. There is only the hardcoded "default" store right now.

@nitedani
Copy link
Member Author

nitedani commented Jan 2, 2024

Added StoreHookOnly and StoreApiOnly types.
StoreHookOnly is only the function,
StoreApiOnly is only the getState|setState|subscribe|destroy.

It's not that misleading anymore. (maybe useStoreApi(useStore))

import { create, useStoreApi } from 'vike-react-zustand'

interface Store {
  counter: number
}

const useStore = create<Store>((set, get) => ({counter: 1})) // StoreHookOnly<Store>

useStore.getState // Property 'getState' does not exist on type 'StoreHookOnly<Store>'

useStore() // Store
useStore(s => s.counter) // number 

const api = useStoreApi(useStore) // StoreApiOnly<Store>

api.getState() // Store

api() // This expression is not callable.

@@ -33,7 +33,7 @@ export default function VikeReactZustandWrapper({ pageContext, children }: VikeR
for (const [key, store] of stores) {
// Trick to make import.meta.env.SSR work direclty on Node.js (without Vite)
// @ts-expect-error
import.meta.env ??= { SSR: true }
import.meta.env = { SSR: true }
Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't work, import.meta.env is not statically evaluated in client code in dev, it would set SSR:true in browser.

Copy link
Member

@brillout brillout Jan 7, 2024

Choose a reason for hiding this comment

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

I didn't test it so I may be wrong, but I'm thinking it should work because import.meta.env.SSR is statically removed (enabling code splitting) whenever Vite transpiles code. I.e. the runtime value import.met.env.SSR is only ever used for externalized server-side.

Copy link
Member

Choose a reason for hiding this comment

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

import.meta.env is not statically evaluated in client code in dev

Ah, sorry, didn't read that. Ok, let's revert then.

@harrytran998
Copy link

@nitedani Hi Niteda, does Vike have any problem merging your PR? I think we are still waiting for you.

@brillout
Copy link
Member

brillout commented Jul 7, 2024

It's the other way around: this PR is blocked by a Vike feature we didn't implement yet.

we are still waiting for you

Sponsoring welcome in case your company would be interested in getting higher priority feature requests with an ETA.

```

> [!NOTE]
> The 3. step will be unnecessary in the future, when Vike extensions can add Vite plugins.
Copy link
Member Author

@nitedani nitedani Jul 7, 2024

Choose a reason for hiding this comment

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

How about this? We can merge it like this and update the readme in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Ok!

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.

3 participants