[Discussion] universal-middleware/hono

[fortezhuo]

I still don't get about universal middleware.

I have code like this using hono

export const preference = (): MiddlewareHandler => {
  return async function preference(c, next) {
    const cookiePreference = getCookie(c, "preference")
    const preference = cookiePreference
      ? JSON.parse(cookiePreference)
      : PREFERENCE
    c.set("preference", preference)
    await next()
  }
}

export const ssr = () =>
  vikeNode({
    pageContext: (c: Context) => {
      return {
        preference: c.var.preference,
        user: c.var.user,
        vendor: c.var.vendor,
      }
    },
    static: {
      root: distClient,
    },
  })

instance.use(preference())
instance.use(ssr())

And with universal-middleware, can I still use hono context using native behavior like c.var and c.set ?

[brillout]

@fortezhuo Does magne4000/universal-middleware#75 answer your question?

@magne4000 Maybe we can start documenting runtime at https://universal-middleware.dev/recipes/context-middleware.

It's currently undocumented because I'm still not sure what the actual user-facing API should look like for it.

Happy to help if you feel like you need a fresh perspective on it.

[fortezhuo]

Hm.. I still don't get how to use native behavior like c.var and c.set ? Maybe documentation with more examples will be great for better understanding

[magne4000]

@fortezhuo the "right" way to do that is with universal-middleware all the way:

import { type Get, type UniversalMiddleware } from "@universal-middleware/core";
// This lacks documentation, I'll fix this
import { createMiddleware } from "@universal-middleware/hono";
import { getCookie } from "@universal-middleware/core/cookie";

const preference = (() => (request, context, runtime) => {
  // Update Context
  return {
    ...context,
    preference: getCookie(request, "preference"),
    // ...
  }
}) satisfies Get<[], UniversalMiddleware>;

// universal Context is automatically applied to `pageContext`
instance.use(createMiddleware(preference)())

NOTE: c.var is currently not available in runtime, but I can easily add it if you need to @fortezhuo

It's currently undocumented because I'm still not sure what the actual user-facing API should look like for it.

Happy to help if you feel like you need a fresh perspective on it.

@brillout Started discussion magne4000/universal-middleware#76

Maybe documentation with more examples will be great for better understanding

👍 You're absolutely right. I'm on it

[brillout]

the "right" way

If the user wants to stay flexible about his server framework choice, then I agree.

If the user loves hono and wants everything to be hono, then I guess the right way is to always use Hono utilities instead of universal-middleware utilities? Will he still be able to deploy anywhere (if that isn't the case then it'd be great if we eventually teach the user about it)?

[magne4000]

If the user loves hono and wants everything to be hono, then I guess the right way is to always use Hono utilities instead of universal-middleware utilities?

I guess so? That would be a good reason to add back pageContext then. I don't really like it, but it kinda make sense, as it doesn't require users to learn about universal-middleware.

Will he still be able to deploy anywhere (if that isn't the case then it'd be great if we eventually teach the user about it)?

I think that if the chosen server can be deployed somewhere, using non universal middlewares shouldn't cause any issue.

[brillout]

I guess so? That would be a good reason to add back pageContext then. I don't really like it, but it kinda make sense, as it doesn't require users to learn about universal-middleware.

I was mentioning this a little bit last time we talked about it: my idea is that once vikejs/vike#1268 is implemented the user will never have to add things to pageContextInit anymore. Because it will include everything. The user will still be able to add initial props to pageContext by using the upcoming +onBoot and/or +onRequest hooks.

So I think we don't need to revive the pageContext() option.

I think that if the chosen server can be deployed somewhere, using non universal middlewares shouldn't cause any issue.

I guess Hono can be deployed anywhere. But Express.js cannot (and/or shouldn't) so maybe we should, eventually, add some kind of warning for Express.js in the future (maybe in Bati).

[magne4000]

[...] add some kind of warning for Express.js in the future (maybe in Bati).

I second that, a "portable" tag (like we have for "bleeding edge") could be great.

[brillout]

"portable" tag

Love it.

[brillout]

NOTE: c.var is currently not available in runtime, but I can easily add it if you need to

Maybe we should systematically add everything 🤔 I guess it's always nice if the user can access everything, otherwise universal-middleware may hinder him.

[fortezhuo]

I think that if the chosen server can be deployed somewhere, using non universal middlewares shouldn't cause any issue.

Thanks for reply. Currently I fallback and use 0.1.x version instead, and I hope using universal-middleware can be set as experimental feature. So user can choose using experimental feature or stay stable

[brillout]

Currently I fallback and use 0.1.x version instead, and I hope using universal-middleware can be set as experimental feature. So user can choose using experimental feature or stay stable

@magne4000 WDYT?

[brillout]

@fortezhuo We will revive the pageContext option.

[magne4000]

@fortezhuo pageContext function is back in 0.2.4. You can use it as in 0.1.x

[fortezhuo]

NOTE: c.var is currently not available in runtime, but I can easily add it if you need to @fortezhuo

I try to bump version vike-node@latest, and still got error. And I assume the error caused by missing context in runtime

export const ssr = () =>
  vikeNode({
    pageContext: (c: Context) => {
      return {
        preference: c.var.preference,
        user: c.var.user,
        vendor: c.var.vendor,
      }
    },
    static: {
      root: distClient,
    },
  })

[magne4000]

runtime.hono is your Context now:

export const ssr = () =>
  vikeNode({
    pageContext: (runtime: RuntimeAdapter) => {
      const c = runtime.hono;
      return {
        preference: c.var.preference,
        user: c.var.user,
        vendor: c.var.vendor,
      }
    },
    static: {
      root: distClient,
    },
  })

[brillout]

@magne4000 How about this:

 app.use(
   vike({
-    pageContext(runtime: RuntimeAdapter) {
+    pageContext(runtime) {
       return {
         user: runtime.req.user
       }
     }
   })
 )

And we add a note mentioning what runtime is and that explains it a bit.

Alternatively, we add a page at universal-middleware's docs that explains RuntimeAdapter, add a link to it in vike-node's readme, then revive the note I removed that was mentioning universal-middleware.

[magne4000]

And we add a note mentioning what runtime is and that explains it a bit.

We'll start with this. If the need arises, I'll add those details in universal-middleware doc too.

[magne4000]

Actually it will either bloat the README, or be too much confusing that way, I'll document RuntimeAdapter in universal-middleware.

[magne4000]

Note added with link to universal-middleware doc https://github.com/vikejs/vike-node?tab=readme-ov-file#custom-pagecontext

[brillout]

vike-node uses universal-middleware and automatically adds the universal context to pageContext.

Maybe we can make it a separate second note Note that `runtime` is already available at `pageContext.runtime` [...] for example:.

[magne4000]

vike-node uses universal-middleware and automatically adds the universal context to pageContext.

Maybe we can make it a separate second note Note that `runtime` is already available at `pageContext.runtime` [...] for example:.

That is incorrect confusing, because context != runtime, and explaining the Context here would be more confusing I think. If the user is curious, they can take a look at universal-middleware doc.

FYI if you have a middleware returning some JS object like { a: 1, b: 2 }, it becomes the new context for subsequent middlewares. So in this case, after merging this context into pageContext you would have pageContext.a === 1 and pageContext.b === 2.

EDIT: pageContext.runtime is indeed also added in addition to the whole universal context See #26 (comment)

[brillout]

Assuming the user doesn't directly use universal-middleware, then maybe we can omit mentioning automatically adds the universal context to pageContext. Maybe we can only mention pageContext.runtime then.

[magne4000]

Assuming the user doesn't directly use universal-middleware, then maybe we can omit mentioning automatically adds the universal context to pageContext. Maybe we can only mention pageContext.runtime then.

Removing the ref to the universal Context would be less confusing indeed.

But I just realized that currently, the universal context AND the runtime are spread into the pageContext.
The runtime should indeed probably be behind its own key. WDYT?

[brillout]

Hm, good question. Maybe both... it's kind of the philosophy behind vikejs/vike#1268. But, yea, it's probably easier if we start with pageContext.runtime and flatten it later.

[magne4000]

Would you rather wait for the next breaking change to add this refacto (as it's also a breaking change, even if not properly documented)? I think that can wait, and I can just remove the universal context reference for now

[brillout]

Personally, I think we can introduce that little breaking change as a patch, without bumping the minor. I don't think any user uses it. Actually, we could argue we didn't mention it so far so it wasn't part of the stable api 😁 But as you wish, no hard preferences on this.

[magne4000]

I don't think anyone uses it either, so #33

[magne4000]

0.2.8 released with pageContext.runtime