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

Core: Expose Server instance through the pluggable Builder API #14468

Merged
merged 2 commits into from
Apr 10, 2021

Conversation

eirslett
Copy link
Contributor

@eirslett eirslett commented Apr 4, 2021

This PR mirrors the changes in SasanFarrokh@b6f5bb2
where Vite needs to hook up its websocket to Storybook's underlying http.Server instance in order to support HMR.

I just want some input from @SasanFarrokh and @patak-js. Once we expose Storybook's underlying http.Server through the API, there's no going back (because of backwards compatibility in the API). Is there any way we can connect Vite's HMR websocket to Storybook's underlying dev server, purely through the express.Router that is already provided in the existing Builder API? Or is exposing the http.Server the best option?

Builder implementations (e.g. Vite) may have hot module replacement
implementations based on Websockets. The websocket has to be mounted
on the Server instance - which is why the builder needs access to
it from the API.
@shilman shilman changed the title Expose the Server instance through the pluggable Builder API Core: Expose Server instance through the pluggable Builder API Apr 10, 2021
@shilman
Copy link
Member

shilman commented Apr 10, 2021

Change looks clean but I'd like to hear back from the Vite side to see if there's any way we can do this with the router per your description comment. @SasanFarrokh @patak-js

cc @ndelangen @tmeasday

@patak-dev
Copy link

@eirslett discussed in Vite Land with other members that have more experience regarding integrating Vite with other tools. So I trust his assessment regarding the need for this PR.

FWIW, I think that exposing the server is a good idea independently of its usage in Vite. As explained in this PR vitejs/vite#2338, where the config for a custom server for hmr was added to Vite, there are some environments where creating a separate server is not an option.

About the feature request regarding Vite specifically, @eirslett I think it would be good if you could explain a bit more why letting Vite create its own hmr server doesn't work for the StoryBook integration, so it is documented why this is needed in this context.

@eirslett
Copy link
Contributor Author

I think it would be good if you could explain a bit more why letting Vite create its own hmr server doesn't work for the StoryBook integration, so it is documented why this is needed in this context.

I'm not sure how to best explain it... I suppose in theory Vite could create a dev server of its own, completely independent of Storybook's own server, but then we would have to listen on two separate HTTP ports, one for Storybook's dev server and another one for Vite's. It would mean configuring two ports, or assigning a random available port and coordinating between the two servers. It's just so much better to integrate Storybook and Vite on the same dev server.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Going to merge this. If we end up not going this route, let's revert it before 6.3 is released to reduce the surface area.

@shilman shilman merged commit 149bc36 into storybookjs:next Apr 10, 2021
@eirslett eirslett deleted the builder-should-expose-server branch April 12, 2021 15:00
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.

None yet

3 participants