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

[Bug]: 7.0.0-beta.28 HMR not working with @storybook/react-vite #20643

Closed
stevezhu opened this issue Jan 17, 2023 · 17 comments
Closed

[Bug]: 7.0.0-beta.28 HMR not working with @storybook/react-vite #20643

stevezhu opened this issue Jan 17, 2023 · 17 comments

Comments

@stevezhu
Copy link

stevezhu commented Jan 17, 2023

Describe the bug

HMR doesn't seem to be working with storybook react with the vite builder.

Package version: @storybook/[email protected]

Wondering if anyone has anymore context on why the following options are set for the vite builder dev server config:
Reference: https://github.com/storybookjs/storybook/blob/v7.0.0-beta.28/code/lib/builder-vite/src/vite-server.ts#LL18-L21

hmr: {
  port: options.port,
  server: devServer,
},

It seems to work fine if the vite hmr options are removed which allows vite to initialize its own websocket server for hmr.

With the existing config, the websocket server doesn't seem to be able to receive the upgrade event below.
Reference: https://github.com/vitejs/vite/blob/v4.0.4/packages/vite/src/node/server/ws.ts#L99-L105

wsServer.on('upgrade', (req, socket, head) => {
  if (req.headers['sec-websocket-protocol'] === HMR_HEADER) {
    wss.handleUpgrade(req, socket as Socket, head, (ws) => {
      wss.emit('connection', ws, req)
    })
  }
})

To Reproduce

No response

System

Environment Info:

  System:
    OS: macOS 13.1
    CPU: (8) x64 Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
  Binaries:
    Node: 19.4.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.2.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 108.0.5359.124
    Edge: 109.0.1518.52
    Firefox: 85.0.2
    Safari: 16.2

Additional context

No response

@thebuilder
Copy link
Contributor

Also noticing that HMR broke after upgrading. I've been testing different versions, and seems like it broke with beta.26 - Maybe it's caused by #20594 ?

Changing a component causes it to skip printing the invalidate message:
In beta.25:

2:43:13 PM [vite] hmr update /src/modules/Footer/Footer.tsx, /src/styles/globals.css
2:43:13 PM [vite] hmr invalidate /src/modules/Footer/Footer.tsx
2:43:13 PM [vite] hmr update /src/styles/globals.css, /virtual:/@storybook/builder-vite/vite-app.js

In beta.26:

2:43:13 PM [vite] hmr update /src/modules/Footer/Footer.tsx, /src/styles/globals.css

@stevezhu stevezhu changed the title [Bug]: 7.0.0-beta.28 [Bug]: 7.0.0-beta.28 HMR not working with @storybook/react-vite Jan 17, 2023
@stevezhu
Copy link
Author

stevezhu commented Jan 17, 2023

True I've been able to reproduce the same thing between beta.25 and beta.26.

Yeah it does seem like the only relevant change made between those versions is #20594.

@stevezhu
Copy link
Author

It looks like it actually says there are issues with HMR in the readme for this plugin:
https://github.com/crcong/vite-plugin-externals

Warning: If you loaded production library in vite dev mode , may make HMR fail.

You can use disableInServe: true option to avoid transform in serve mode.

@thebuilder
Copy link
Contributor

See #20655

@shilman
Copy link
Member

shilman commented Jan 18, 2023

w00t!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.30 containing PR #20654 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Jan 18, 2023
@stevezhu
Copy link
Author

stevezhu commented Jan 19, 2023

It seems like with the new version (v7.0.0-beta.30) I get this when opening the storybook preview:

1:33:27 AM [vite] Internal server error: Failed to resolve import "@storybook/preview-api" from "../../../../../../virtual:/@storybook/builder-vite/vite-app.js". Does the file exist?
  Plugin: vite:import-analysis
  File: /virtual:/@storybook/builder-vite/vite-app.js:1:56
  1  |  import { composeConfigs, PreviewWeb, ClientApi } from '@storybook/preview-api';
     |                                                         ^
  2  |    import '/virtual:/@storybook/builder-vite/setup-addons.js';
  3  |    import { importFn } from '/virtual:/@storybook/builder-vite/storybook-stories.js';
      at formatError (file:///<projectDir>/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:41235:46)
      at TransformContext.error (file:///<projectDir>/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:41231:19)
      at normalizeUrl (file:///<projectDir>/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:39540:33)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async TransformContext.transform (file:///<projectDir>/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:39673:47)
      at async file:///<projectDir>/node_modules/.pnpm/[email protected][email protected]/node_modules/vite-plugin-inspect/dist/index.mjs:676:23
      at async Object.transform (file:///<projectDir>/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:41506:30)
      at async loadAndTransform (file:///<projectDir>/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-5e7f419b.js:39313:29)

@IanVS
Copy link
Member

IanVS commented Jan 19, 2023

@valentinpalkovic @ndelangen any idea why this would now be failing in pnpm projects? I thought we had all the dependencies straightened out even before the pre-bundling, so not sure why this is breaking now.

@stevezhu does it still work correctly when building for production?

@ndelangen
Copy link
Member

It's literally a dependency of the vite-builder?!

"@storybook/preview-api": "7.0.0-beta.29",

@IanVS
Copy link
Member

IanVS commented Jan 19, 2023

Yes, but with pnpm (and maybe yarn pnp), it needs to also be imported in the right context. Because this is being loaded in the browser, we need to make sure it's in the user's package.json. I remember now, previously we imported these internal packages from the framework, which re-exported from preview-api, etc. Now we rely on the pre-bundling, which is turned off in dev now, so it's breaking. I am guessing that it still works fine in builds.

@stevezhu
Copy link
Author

@valentinpalkovic @ndelangen any idea why this would now be failing in pnpm projects? I thought we had all the dependencies straightened out even before the pre-bundling, so not sure why this is breaking now.

@stevezhu does it still work correctly when building for production?

It looks like it's related to this issue it was mentioned in storybookjs/builder-vite#55 (comment). I was using pnpm and the hoisting workaround mentioned does work for me. Seems fine with the production build.

Also trying to look into why vite-plugin-externals is breaking HMR from the websocket upgrade route.

@Dschungelabenteuer
Copy link
Member

Dschungelabenteuer commented Jan 19, 2023

If I understand correctly, this is only blocked for now until #20655 is figured out and a fix is released on vite-plugin-externals's side? I would really like to avoid that shameful hoisting workaround, but I do not really want to downgrade either because I'd like to benefit from the most recent releases of 7.0 beta.

Comparing both vite-plugin-externals and the former rollup-plugin-external-globals, and the transformed outputs in the virtual vite-app file, it looks like we could (at least for now) possibly achieve the same behavior in a very simple(r) and naive way by creating a dummy plugin inside builder-vite? Let me setup a draft PR quickly, just to illustrate what I mean.

@stevezhu
Copy link
Author

stevezhu commented Jan 20, 2023

I finally figured out the issue after an unreasonable amount of debugging on bundled vite server code 😂. The vite-plugin-externals plugin isn't properly following the vite api for modifying config. Fix here: crcong/vite-plugin-externals#27. After the fix is published, #20655 could probably be merged in to revert the addition of disableInServe: true.

Could make sense to move to any internal plugin too if the plugin is as simple as #20698, but otherwise this should fix the issue.

If anyone is curious, it's because plugin configs are deeply merged by vite in the config hook.

conf = mergeConfig(conf, res) 

https://github.com/vitejs/vite/blob/v4.0.4/packages/vite/src/node/config.ts#L1109

Since vite-plugin-externals was passing in the entire config to be merged, the merge code was recursively merging the hmr server from @storybook/builder-vite with itself.

hmr: {
  port: options.port,
  server: devServer,
},

https://github.com/storybookjs/storybook/blob/v7.0.0-beta.29/code/lib/builder-vite/src/vite-server.ts#L20

@stevezhu
Copy link
Author

stevezhu commented Jan 20, 2023

Also @shilman it would be great if you could reopen this issue to track the fix.

@valentinpalkovic
Copy link
Contributor

Thank you so much for your time debugging this! 🎉I will ask internally, whether we want to wait for a fix of vite-plugins-externals, or moving that plugin internally.

FYI: @ndelangen, @IanVS

@stevezhu
Copy link
Author

stevezhu commented Jan 20, 2023

Yeah not sure if the maintainer of the package will respond though it seems like they haven't really been active since july.

@IanVS
Copy link
Member

IanVS commented Jan 20, 2023

I sent them an email as well this morning. But yes, I think if we don't get a response in a couple of days we'll need to take a different approach.

@shilman shilman moved this from Nice to have to Required for GA in Core Team Projects Jan 23, 2023
@shilman
Copy link
Member

shilman commented Jan 28, 2023

¡Ay Caramba!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.36 containing PR #20698 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants