feat: built-in +server support (via Universal Deploy, deprecating vike-photon)#3106
feat: built-in +server support (via Universal Deploy, deprecating vike-photon)#3106
vike-photon)#3106Conversation
packages/vike/src/server/runtime/renderPageServer/createHttpResponse.ts
Outdated
Show resolved
Hide resolved
What's the scope of the current state? Asking because I ain't sure what is temporary or not. |
|
It's feature complete. So the only temporary thing is the remaining TODO comment regarding |
|
E.g. the I guess the goal is to have to distinct response types, either:
I think we can either return only 1 or only 2. Not sure whether returning both would make sense. |
|
Actually, I'm realizing maybe it could make sense: using UM internally to add support for +middlewares with all the current |
packages/vike/src/server/runtime/renderPageServer/createHttpResponse.ts
Outdated
Show resolved
Hide resolved
6238f15 to
560107a
Compare
cdbf593 to
234f499
Compare
|
FYI did some minor refactoring 234f499 — should be equivalent. Bonus: the PR diff is now a lot more readable. |
|
Let me do a final round of reviewing and then I'm good on my side. |
packages/vike/src/server/runtime/renderPageServer/createHttpResponse.ts
Outdated
Show resolved
Hide resolved
|
LGTM (except #3106 (comment)) |
c109c93 to
6767498
Compare
vike-photon features (via Universal Deploy)
# Conflicts: # packages/vike/package.json # pnpm-lock.yaml
# Conflicts: # pnpm-lock.yaml
I think we should keep it as it is for now. At the moment, the main goal of UD isn't to be zero-config, but rather to support as many use cases as possible with as little code as possible. Adding configuration options like That might make it harder to present UD to developers from other frameworks, and in practice it doesn't really change anything for us. |
Yes, it's just that, or even just using the |
In UD, I'll keep the |
|
I'd frame UD (e.g. in the README) as:
The key word here is swiss army knife. From that perspective, I think it makes sense to add conveniences to UD to make it zero-config. If we position it as zero-config that makes UD more appealing and more trustworthy, because we signal that we're open to add features to ease the life of framework developers. As a framework author, I don't care that much whether UD is 10% more or less code, but I will very much appreciate an easy integration. Another way to frame UD:
I think that's an appealing "sales pitch". It revolves around making the life of the framework author easier. The fact that UD is 10% more or less code doesn't impact the sales pitch. That's how I see it. I don't think adding options to UD doesn't have to be zero-config for every framework (we can tell them what to do), but it should at least be zero-config for UD's flagship integration example. Showing how easy it was to add UD to Vike makes it very appealing, I think. |
I agree internally the more verbose the virtual IDs the better for debuggability. But for public facing virtual IDs, I think it's important we avoid users to think "what the heck is this cryptic ID". 99,9% of users won't know what UD is. That said, it only affects Cloudflare users, so it's not that important either. |
|
@brillout good for a new review 👍 |
|
👍 👀 A bit busy at the moment, but let me know if it's blocking you. |
|
The
export default {
- photon: {
- standalone: {
- bundle: true,
- minify: true
- },
- target: 'my_target'
- }
}I don't know where to migrate them. |
|
Having two server entries, the migration should be:
export default {
- photon: {
- server: process.env.NODE_ENV === 'production'
- ? 'server/index.ts'
- : 'server/entry.node.ts'
- }
}
import { Hono } from 'hono'
-import { apply } from '@photonjs/hono'
+import { addVikeMiddleware } from '@vikejs/hono'
const app = new Hono()
-apply(app)
+addVikeMiddleware(app)
export default app
-import { serve } from '@photonjs/hono'
-import app from './index'
-const port = +(process.env.PORT || 3000)
-export default serve(app, { port })
+import app from '../server/index'
+export default {
+ fetch: app.fetch
+}How can I change the server entry? |
Perhaps this option should be added in |
|
@rtritto Thanks for the feedback! Although keep it mind that this isn't the official API yet. E.g. we'll remove |
|
@rtritto good catch, thanks for letting us know. I'll complete the migration doc. FYI, you should be able to point to different server entries doing something like this: // +config.ts
import devEntry from '/pages/+server'
import prodEntry from '/server/index'
export default {
server: process.env.NODE_ENV === 'production' ? prodEntry : devEntry
}I will not document this particular use case though, as we do not recommend having 2 server entries. Could you share why you currently have those 2 servers entries?
Done. |
In production (deployed at Vercel as serverless SSR), in the entry point ( import app from '../dist/server/index.mjs'
export const GET = app.fetch
export const POST = app.fetchInstead, the serve function (app.listen) is needed only in development. |
In the packages/vike/src/types/Config.ts of this PR, the export default {
server: process.env.NODE_ENV === 'production' ? '/server/index.ts' : '/pages/+server.ts'
} |
I've updated the typing. And actually, I just tested, there correct migration will be: export default {
server: process.env.NODE_ENV === 'production' ? 'import:./server/index.ts:default' : 'import:./pages/+server.ts:default'
} |
WDYT about to add a robust check to be smart? if (!server.startsWith('import:')) {
server = `import:${server}:default`
}By this way, we can use: export default {
server: process.env.NODE_ENV === 'production' ? '/server/index.ts' : '/pages/+server.ts'
} |
TODO doc
test/vite-plugin-verceltests