Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use preview server parameter in preview server hook #11647

Merged
merged 6 commits into from
Mar 25, 2023

Conversation

erxclau
Copy link
Contributor

@erxclau erxclau commented Jan 10, 2023

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@erxclau erxclau closed this Jan 10, 2023
@erxclau erxclau reopened this Jan 10, 2023
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I think this makes sense.

// apply server hooks from plugins
const postHooks: ((() => void) | void)[] = []
for (const hook of config.getSortedPluginHooks('configurePreviewServer')) {
postHooks.push(await hook({ middlewares: app, httpServer }))
postHooks.push(await hook(server))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased on main and pushed some commits 👍

@sapphi-red sapphi-red added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jan 16, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for cleaning this up sapphi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use PreviewServer parameter type in configurePreviewServer hook
4 participants