Chris/vike pinia

[4350pChris]

See vikejs/vike#1374

I decided to follow the other repositories and create a package folder for this. We should probably move the existing code into that folder as well, but for now I let it be so as not to have too many changes.

I'd appreciate general feedback on my approach here - is having new hooks the way to go here? Is it ok to provide a default onCreateApp hook here and expect the user to provide their own, while importing the one from this package like in the example +onCreateApp.ts?

I do have one issue with this implementation - other stores / data fetching tools like tan-query need to hook into the same place, which becomes unwieldy. So my solution would be to make the hooks I introduced more general - maybe call them onAfterRenderSSRApp (dehydrate) and onBeforeMountApp (hydrate) or something. But then I run into two issues - firstly, onAfterRenderSSRApp would have to return an object that we add to the pageContext that onRenderHtml returns, as we need to leave it up to the package what the property is called that will be sent to the client, otherwise we need to include it in vike-vue's passToClient. That would, however, expose another low-level hook that can be used to directly set properties on pageContext. Secondly, we don't want to exclude users from using multiple state management libraries. I for myself use both pinia and vue-query. If they both use the aforementioned hooks, we'd need some way of orchestrating that or go and make this a proper pub / sub where any function can register to get notified by these hooks. In general I think it would be very beneficial if we don't have to touch vike-vue's base package for any other state management tool, as no normal user can do this, thus preventing any packages outside of this repository to be implemented in the same way.

I hope this is written in a way that's not too confusing.

Also I'll try to write some tests when I find the time.

[lourot]

Thanks a lot for all your great contributions @4350pChris!

We should probably move the existing code into that folder as well

Yes I'll do that after we merge this PR.

So my solution would be to make the hooks I introduced more general - maybe call them onAfterRenderSSRApp (dehydrate) and onBeforeMountApp (hydrate) or something.

Yes that sounds good.

as we need to leave it up to the package what the property is called that will be sent to the client

What about we add to passToClient a generic name like fromHtmlRenderer? Then any onAfterRenderSSRApp() can return a { foo: 'bar', initialStoreState: something } that will just be put in the pageContext as pageContext.fromHtmlRenderer.foo and pageContext.fromHtmlRenderer.initialStoreState. What do you think?

[lourot]

One more thing @4350pChris: if you could avoid force-pushing to open pull-requests it would be great. It's really hard to keep track of what has been reviewed already and what not if the history changes. On top of that, if one of us pushes something exactly before you force-push, the commit will just silently get lost and either we won't notice or it will require significant brain-power to reconcile histories. Thanks 🙏

[4350pChris]

One more thing @4350pChris: if you could avoid force-pushing to open pull-requests it would be great. It's really hard to keep track of what has been reviewed already and what not if the history changes. On top of that, if one of us pushes something exactly before you force-push, the commit will just silently get lost and either we won't notice or it will require significant brain-power to reconcile histories. Thanks 🙏

Alright. I didn't want to pollute the commit history, and no one had reviewed it so I figured I'd use the opportunity to keep it clean. But I see your point. Maybe I'll just defer PRs until they're more mature.

[4350pChris]

Thanks a lot for all your great contributions @4350pChris!

We should probably move the existing code into that folder as well

Yes I'll do that after we merge this PR.

So my solution would be to make the hooks I introduced more general - maybe call them onAfterRenderSSRApp (dehydrate) and onBeforeMountApp (hydrate) or something.

Yes that sounds good.

as we need to leave it up to the package what the property is called that will be sent to the client

What about we add to passToClient a generic name like fromHtmlRenderer? Then any onAfterRenderSSRApp() can return a { foo: 'bar', initialStoreState: something } that will just be put in the pageContext as pageContext.fromHtmlRenderer.foo and pageContext.fromHtmlRenderer.initialStoreState. What do you think?

Great idea.

[lourot]

@4350pChris I have made changes around types and am now happy with this PR. Let me know what you think.

[lourot]

I'm running out of time for today so I'll resume tomorrow but it seems like pnpm is behaving differently on CI and on my system:

• on CI it produces packages/vike-pinia/dist/renderer/+config.js and it seems it is also how it behaves on your system @4350pChris
• on my system it produces packages/vike-pinia/dist/+config.js and thus my recent changes in exports, removing renderer/

I see in package.json we indicate [email protected] but on my system (Ubuntu) I have 8.10.5 installed. It could be the reason for the difference.

[4350pChris]

@AurelienLourot It actually does the same thing for me too. I'm a little confused tight now. Let me change it to the way it is for the both of us. I'd expect CI to behave in the same way actually.

[4350pChris]

LGTM?

[lourot]

Yes I'm also a bit confused now as to what happened but it's also working fine for me now too. Thanks a lot. Merging.

We should probably move the existing code into that folder as well

And I'll also do this right away.