vikejs / vike-vue

🔨 Vue integration for Vike
https://vike.dev/vike-vue
MIT License
34 stars 5 forks source link

Add onCreateApp hook #63

Closed 4350pChris closed 5 months ago

4350pChris commented 5 months ago

close #60

This adds a hook onCreateApp that is invoked when the app is created, both on the server and the client.

Two more things:

  1. Why do both onRenderClient and onRenderHtml use the vuePlugins hook? Couldn't that live inside app.ts just like the code in this PR?
  2. This hook overlaps conceptually with the vuePlugins hook, which can still be used to have a shortcut to defining plugins. However, some people might use that hook while others might use onCreateApp and then do app.use(plugin) in there. Should we just leave it up to the user in this case and accept a little bit of fragmentation?
brillout commented 5 months ago

Neat :100:

I agree, the fragmentation isn't ideal. How aout we add a little console.warn('[Warning] +vuePugins.js is deprecated, use +onCreateVue() instead')?

Also, how about we add app as pageContext.app? So that the user can do onCreateApp({ app }) or onCreateApp(pageContext) if he needs more context.

  1. Why do both onRenderClient and onRenderHtml use the vuePlugins hook? Couldn't that live inside app.ts just like the code in this PR?

Indeed, this seems redundant. I'd be 👍 for refactoring that bit in this PR.

4350pChris commented 5 months ago

I just tried adding app to pageContext but the compiler complains about some deeply nested types not matching. I assume this is because we wrap pageContext with reactive, which in turn probably changes something around Vue's reactivity system. Apart from that I have reservations about doing this since we're introducing a circular dependency.

const ctx = app.inject(ctxKey)
const sameCtx = ctx.app.inject(ctxKey)
// and so on

So for now I think we should probably just live with two parameters for this function.

brillout commented 5 months ago

If we exclude pageContext.app from the reactive pageContext clone, then I believe we can make it work?

Another added benefit of setting pageContext.app is that it will be available to other hooks as well. So I'm thinking this is a neat addition.

brillout commented 5 months ago

Also, about the deprecation, I was thinking of showing the warning while preserving the code, so that we don't break existing apps? WDYT? Alternatively, we can remove it and bump the semver minor.

4350pChris commented 5 months ago

I did preserve the code - it was moved to app.ts

Also, yeah excluding it from the reactive context would make it work, I did think about that too. However, I was unsure how to type this since we'd need to be clear about where app is available and where it isn't.

Maybe I could add it only to the callback's type, so it says (pageContext: PageContext & { app: App } ? But then that would kind of break when users explicitly set the type for the parameter like onCreateApp(pageContext: PageContext) as they would remove the type reference to app again.

With that being said, I do prefer how the API looks when app is included in pageContext so I'm up for making this work. What do you think?

brillout commented 5 months ago

We can add the type of app to the Vike.PageContext interface at vike-vue/renderer/types.ts as an optional property, I think that would do the trick?

I did preserve the code - it was moved to app.ts

Ah, indeed, sorry about that.

4350pChris commented 5 months ago

I'll implement it like that for now as I can't think of a better way. However, it still has the drawback that in those hooks pageContext.app is typed as optional, when we actually know it's there. On the other hand, any time we use usePageContext in our app, we know it won't be defined. I feel like there should be a way to make this distinction clear on a type level.

brillout commented 5 months ago

Maybe I could add it only to the callback's type, so it says (pageContext: PageContext & { app: App }

This seems like a viable option.

But then that would kind of break when users explicitly set the type for the parameter like onCreateApp(pageContext: PageContext) as they would remove the type reference to app again.

I think it's fine: the user wouldn't use OnCreateAppSync then.

brillout commented 5 months ago

There are some hooks where it will be there, e.g. onHydration(). So I'm inclined to make it part of Vike.PageContext.

The issue is that Vike doesn't have an option right now to refine hooks individually. So I'm not sure there is a better solution.

4350pChris commented 5 months ago

I think the way it is now, I'm pretty happy with it. Lmk what you think and from my side LGTM

brillout commented 5 months ago

LGTM. @AurelienLourot WDTY?

brillout commented 5 months ago

@4350pChris I did a little bit of polishing, let me know if you object with any of it.

4350pChris commented 5 months ago

Looks good 👍