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

Fix bail not being defined for the vite builder #19405

Merged

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Oct 8, 2022

Issue: #19349

Equivalent to storybookjs/builder-vite#510

What I did

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@joshwooding joshwooding requested a review from IanVS October 8, 2022 18:51
@IanVS
Copy link
Member

IanVS commented Oct 9, 2022

This looks good, but let's see how #19406 plays out, because we might not need this change.

@JReinhold
Copy link
Contributor

@joshwooding @IanVS in #19406 I agreed with @ndelangen that we would keep the bail in the return of preview builders because we expect to use them later when we want to shutdown the server more gracefully. But we didn't think the direct export of bail was necessary as it was only used in tests, but judging from the issue this solves I don't think that's correct.

So in short, we'll keep bail for now and this PR can move on.


export async function bail(e?: Error): Promise<void> {
try {
return await server.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: In theory server can be undefined here - although I can't see if it can in practice right now.

To be defensive I'd change the type to be let server: ViteDevServer | undefined; and add optional chaining when using it.

Copy link
Member

@IanVS IanVS Oct 12, 2022

Choose a reason for hiding this comment

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

Would you want that in order to avoid the console warning? (this is in a try/catch.) I think it might be nice to show it, so we know what's happening, but it's also not super actionable, so I could go either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm good point, I actually have no strong preference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this but I chose to leave it unguarded so that it would throw and error.

@IanVS IanVS merged commit 6c907eb into storybookjs:next Oct 13, 2022
@joshwooding joshwooding deleted the fix-bail-being-undefined-for-vite-builder branch October 13, 2022 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants