Closed KhodeN closed 1 year ago
Can confirm, the WSS port and host must be configurable individually and independent from the server host and port!
If sb runs behind a proxy / load balancer, you might want the server to run on localhost:8080. That does not mean that WSS requests made by the browser should go to wss://localhost:8080. Instead you want your proxy / loadbalancer here, e.g. wss://storybook.domain:443 which then redirects traffic internally to the storybook server on 8080.
I can confirm this, too. I would like to use Storybook in a Cloud IDE (Gitpod or Github Codespaces), but WSS needs to be configurable for that.
cc @tmeasday
Seems like a simple change to read host
from options
in getServerChannelUrl
if set. Would someone like to send a PR?
I think we should not only make it configurable, but also give it a better default (not just a fixed localhost). For example, in Next.js it is also not configurable, but has a nice default (at least I never had trouble with it). I took a quick look at the source code of how they do it (simply searched for "wss"):
export function getSocketProtocol(assetPrefix: string): string {
let protocol = window.location.protocol
try {
// assetPrefix is a url
protocol = new URL(assetPrefix).protocol
} catch (_) {}
return protocol === 'http:' ? 'ws' : 'wss'
}
export function useWebsocket(assetPrefix: string) {
const webSocketRef = useRef<WebSocket>()
useEffect(() => {
if (webSocketRef.current) {
return
}
const { hostname, port } = window.location
const protocol = getSocketProtocol(assetPrefix)
const normalizedAssetPrefix = assetPrefix.replace(/^\/+/, '')
let url = `${protocol}://${hostname}:${port}${
normalizedAssetPrefix ? `/${normalizedAssetPrefix}` : ''
}`
if (normalizedAssetPrefix.startsWith('http')) {
url = `${protocol}://${normalizedAssetPrefix.split('://')[1]}`
}
webSocketRef.current = new window.WebSocket(`${url}/_next/webpack-hmr`)
}, [assetPrefix])
return webSocketRef
}
Yes, using host and port based on window.location
like @medihack suggests would also solve the deployed-behind-proxy-issue I had described.
If you're wondering why a dev server is being deployed behind a proxy: We use kubernetes also for local development, so for development the dev server is being deployed, for testing & prod an nginx. I guess it's not a too unusual scenerio these days.
@ndelangen what are your thoughts on this change (calculating server URLs in the browser, rather than setting them in the build)?
Why would the server channel be used in a deployed storybook? It's only supposed to be used in dev-mode?
There should be no harm in making it use the same origin?
@ndelangen I don't think we are talking about "deployed" SBs here, but instead in-development SBs that are hosted online, for instance like GitHub codespaces type situations.
aha, sure. seems allright to me to change that to match the current origin 👍
Are there no workarounds so far?
Are there no workarounds so far?
I have used the https://www.npmjs.com/package/patch-package for patched the code directly (write my own host). It's seems stupid, but works)
Are there no workarounds so far?
I have used the https://www.npmjs.com/package/patch-package for patched the code directly (write my own host). It's seems stupid, but works)
Not a too bad idea, when using yarn, patch functionality is even built in. I forget about this constantly.
@KhodeN @krossekrabbe A blog post (or even a gist) about how you implemented this workaround would be immensely useful to folks in the community who are trying to advocate internally for remote development environments such as GitHub Codespaces 🙏🏻
Can we have a real fix before the final release of v7? The fix looks so easy (just use window.location
), and it affects anyone working in a cloud IDE.
I think the issue is here: https://github.com/storybookjs/storybook/blob/441338a6aeddc0f6f7e4b945ca773ff5ba19875f/code/lib/core-server/src/utils/server-address.ts#L19-L21
We pass this value to the manager, the full URL.
I think the issue is here:
We pass this value to the manager, the full URL.
Exactly, and this would fix it
export const getServerChannelUrl = () => {
const { hostname, port, protocol } = window.location
return `${protocol === 'https:' ? 'wss' : 'ws'}://${hostname}:${port}`
}
I think It doesn't need to be configurable as it is client-side only (but this could also be easily implemented).
That code is called in node, which has no window
That code is called in node, which has no
window
You are right. Sorry for the quick shot :-( It is indeed used on the server by build-dev.ts and then given to the client as SERVER_CHANNEL_URL. Maybe it would be better to not use that variable on the client then, but the window.location
as above and leave the server code alone (or make hostname configurable as well there).
The browser needs to know if it should connect to ws://
or wss://
, and it has to know the port..
.. but tracing the code up, the port always matches the current port:
https://github.com/storybookjs/storybook/blob/d2d6f9d0eddef2176da5815296be0eabe5622434/code/lib/core-server/src/build-dev.ts#L66
So we could just drop that part.
And it's likely save to assume the protocol should be wss://
if location.protocol
is https://
.
So it would seem that passing SERVER_CHANNEL_URL
might be really redundant?
I'll look @tmeasday into this discussion.
Tom, do you think we could drop passing the SERVER_CHANNEL_URL to the preview, and instead rely on code like this in the preview itself:
export const getServerChannelUrl = () => {
const { hostname, port, protocol } = window.location
return `${protocol === 'https:' ? 'wss' : 'ws'}://${hostname}:${port}/storybook-server-channel`
}
We could hard-code /storybook-server-channel
, or continue to pass it from the server to the preview still, but I wonder if there's a point in doing so. We could hard-code it in a shared package, so we don't have to change a hard-coded strin in 2 places.
I think that all sounds pretty reasonable.
@medihack would you be open to creating a PR changing the above? I could pair with you on it, if you'd like.
FYI, the PR would not be merged before the 7.0 release, we're in a feature freeze.
@ndelangen Ok, I can have a look and see with what I can come up in the upcoming weeks.
Jiminy cricket!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.1.0-alpha.8 containing PR #22055 that references this issue. Upgrade today to the @future
NPM tag to try it out!
npx sb@next upgrade --tag future
Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.14 containing PR #22055 that references this issue. Upgrade today to the @latest
NPM tag to try it out!
npx sb@latest upgrade
Not sure if it is worth to re-open but the PR #22055 does not take a different base URL into account. So if you would be running with:
...
async viteFinal(config) {
return mergeConfig(config, {
base: '/storybook',
});
},
...
It will fail due https://github.com/storybookjs/storybook/pull/22055/files#diff-3a2950085f45f23c305d6ec255529302b1d70eb2a078a8111a6c9375159898b6R89 not taking this /storybook url into account
@kevinvalk I was able to fix that in nuxt by adding the second rule to this config:
routeRules: {
'/storybook/**': { proxy: { to: `http://localhost:${STORYBOOK_PORT}/**` } },
'/storybook-server-channel/**': { proxy: { to: `ws://localhost:${STORYBOOK_PORT}/**` } }
},
It is still not working properly. Even Github Codespaces is struggling to run Storybook projects. Is there anything we can do about it?
Describe the bug
Code in storybook/code/lib/core-server/src/utils/server-address.ts has the method
with hardcoded host.
This code doesn't work for custom host (and SSL). It generates the errors in console like:
To Reproduce
System
Additional context
No response