feat: use preview server parameter in preview server hook#11647
feat: use preview server parameter in preview server hook#11647bluwy merged 6 commits intovitejs:mainfrom
Conversation
sapphi-red
left a comment
sapphi-red
left a comment
There was a problem hiding this comment.
Thanks for the PR! I think this makes sense.
| const postHooks: ((() => void) | void)[] = [] | ||
| for (const hook of config.getSortedPluginHooks('configurePreviewServer')) { | ||
| postHooks.push(await hook({ middlewares: app, httpServer })) | ||
| postHooks.push(await hook(server)) |
There was a problem hiding this comment.
configurePreviewServer hooks now runs after the server started to listen on the port.
I'm not sure if this is safe or not. But I think it's better to align with the dev server.
How about adding PreviewServerForHook type that has a nullable resolvedUrls property? And then use PreviewServerForHook for the hooks and use PreviewServer for the return type of preview function for compatibility.
There was a problem hiding this comment.
Ah you're right. I think PreviewServerForHook makes sense, but for the next major, maybe we can make it nullable by default so the types are cleaner.
With this change, I think this PR is good too.
There was a problem hiding this comment.
I think we could merge this in 4.3. @sapphi-red if you agree, you can directly commit your suggestion and merge this one. The alignment is quite uncontroversial to me, we shouldn't need a meeting to review this one IMO.
There was a problem hiding this comment.
I rebased on main and pushed some commits 👍
sapphi-red
added
the
p2-nice-to-have
sapphi-red
force-pushed
the
configure-preview-hook-type
branch
from
aab2019 to
f963c90
Compare
bluwy
left a comment
bluwy
left a comment
There was a problem hiding this comment.
Looks great! Thanks for cleaning this up sapphi
4 participants
Description
Closes #11631. The code changes basically follow what I described in the issue's proposed solution.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).