Use `universal-middleware`

magne4000 commented 1 month ago

Notable changes

Use @universal-middleware/compress for compression
All cache related features have been dropped
getPageContext hook is removed. The whole context from universal middlewares is given as a Context to renderPage. This means that the same effect can be achieved by any custom universal middleware.
No more distinction between node and non-node envs (at least for now :crossed_fingers:)

Next steps (not in this PR)

Put cache feature back?
Is it possible to have some generic way to handle HMR websocket connections, perhaps with crossws?

jasonhilldm commented 1 month ago

Hi. I stumbled across this extension while trying to figure out the best way to deploy to Node and it looks awesome. The one issue I have is that I am using Auth.js for authentication and was previously using universal middleware to add the logged in user to the context.

When I use the vike-node extension though, my modified context is not being seen by Vike so I assume it's using a different context. I'm assuming these draft changes will mean my context changes will be seen by Vike via the use of universal middleware...is that right?

Not sure how far away these changes are, but is there another way that I can add the user to the context using regular middleware so that Vike will see that when I am using vike-node?

magne4000 commented 1 month ago

@jasonhilldm I would suggest to wait for this PR to be finished, as it will fix your issue. Nevertheless, you could still retrieve the context for your server using the getContext function of your server inside the pageContext hook. Something like this should do the trick (untested, and probably does not work for all servers):

import { getContext } from "@universal-middleware/hono";

app.use(
  vike({
    pageContext: (req) => getContext(req)
  })
)

jasonhilldm commented 1 month ago

Thank you - that worked perfectly.

magne4000 commented 6 days ago

Migration

[x] Added to BREAKING CHANGE commits

`0.1.x` to `0.2.x`

Caching support removed

app.use(
  vike({
    compress: false,
-     static: {
-       cache: false
-     }
  })
)

`pageContext`

If you were using it to feed universal-middleware context to pageContext, it's now the default behaviour.
Otherwise, you now need to create a universal context middleware and attach it to your server.

app.use(
  vike({
-    pageContext: (req) => ({
-      user: req.user
-    })
  })
)

brillout commented 6 days ago

Add those as BREAKING CHANGES when squash merging

How about we push empty commits that include these BREAKING CHANGE: entries? It's a bit easier for the squasher.

brillout commented 6 days ago

We can force push if you want to edit previous BREAKING CHANGE: entries.

magne4000 commented 6 days ago

BREAKING CHANGE commits updated

magne4000 commented 4 days ago

Seems like we should be good to merge @brillout

brillout commented 4 days ago

After bundling and publishing this middleware as @universal-middleware-examples/tool, one can then use this middleware as follows:

I think it's even more misleading. Many users will wonder "why should I publish my middleware?".

(I guess the docs is automatically copy-pasting real code from examples to the docs. FYI I used to do that as well but I stopped doing it precisely because of things like this.)

brillout commented 4 days ago

A BREAKING CHANGE: is missing mentioning vike-node/connect.

Also:

- BREAKING CHANGE: `getPageContext` has been removed
+ BREAKING CHANGE: the `pageContext` setting has been removed

brillout commented 4 days ago

LGTM otherwise. I've pushed a couple of commits, let me know if you disagree with any of it.

I've mixed feelings about removing the cache. I kinda liked that the compression was being cached, but I ain't sure whether caching should be done inside vike-node (nor compression for that matter). Maybe we could have vike-server-compress and vike-server-cache extensions. Also a vike-plus that automatically adds all kinds of nice-to-haves.

As for this PR, I'm a bit inclined to remove the compression outside of vike-node. But I'm fine if we keep it for now. We'll see how we want to structure everything as we move forward.

magne4000 commented 4 days ago

LGTM otherwise. I've pushed a couple of commits, let me know if you disagree with any of it.

LGTM

I've mixed feelings about removing the cache. I kinda liked that the compression was being cached, but I ain't sure whether caching should be done inside vike-node (nor compression for that matter). Maybe we could have vike-server-compress and vike-server-cache extensions. Also a vike-plus that automatically adds all kinds of nice-to-haves.

As for this PR, I'm a bit inclined to remove the compression outside of vike-node. But I'm fine if we keep it for now. We'll see how we want to structure everything as we move forward.

I think that the caching layer do not belong directly in this package. I not even sure that I find the feature useful (have we measured that it really improves perfs?).

Regarding the compression, no strong feeling about it. For the sake of merging this PR sooner rather than later, I'd say we keep it as-is for now.

brillout commented 3 days ago

A BREAKING CHANGE: is missing mentioning vike-node/connect.

Also:
- BREAKING CHANGE: `getPageContext` has been removed
+ BREAKING CHANGE: the `pageContext` setting has been removed

This seems to be still missing.

brillout commented 3 days ago

I not even sure that I find the feature useful (have we measured that it really improves perfs?).

I see, good point. Indeed computing a cache key isn't gratis either and bloating memory isn't necessarily a good default.

Regarding the compression, no strong feeling about it. For the sake of merging this PR sooner rather than later, I'd say we keep it as-is for now.

Makes sense :+1:

After bundling and publishing this middleware as @universal-middleware-examples/tool, one can then use this middleware as follows:

I think it's even more misleading. Many users will wonder "why should I publish my middleware?".

(I guess the docs is automatically copy-pasting real code from examples to the docs. FYI I used to do that as well but I stopped doing it precisely because of things like this.)

WDYT?

magne4000 commented 3 days ago

A BREAKING CHANGE: is missing mentioning vike-node/connect. Also:
- BREAKING CHANGE: `getPageContext` has been removed
+ BREAKING CHANGE: the `pageContext` setting has been removed
This seems to be still missing.

Hum no, it's there https://github.com/vikejs/vike-node/pull/16/commits/29128b56847aafa2428ebb7d1b0a48fed8e08b0d

magne4000 commented 3 days ago

After bundling and publishing this middleware as @universal-middleware-examples/tool, one can then use this middleware as follows:

I think it's even more misleading. Many users will wonder "why should I publish my middleware?". (I guess the docs is automatically copy-pasting real code from examples to the docs. FYI I used to do that as well but I stopped doing it precisely because of things like this.)

WDYT?

I'm currently working on some universal-middleware doc, I'll see what I can come up with.

brillout commented 3 days ago

I meant this one:

A BREAKING CHANGE: is missing mentioning vike-node/connect.

magne4000 commented 3 days ago

https://github.com/vikejs/vike-node/pull/16/commits/2a4be82563c8cef9536c37bc12ccca44107a2935

brillout commented 3 days ago

Released as 0.2.0 :rocket:

brillout commented 3 days ago

I made a couple of improvements to the readme, see the last commits. Let me know if you disagree with anything.

magne4000 commented 3 days ago

I made a couple of improvements to the readme, see the last commits. Let me know if you disagree with anything.

Thanks! LGTM

vikejs / vike-node