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

server.handleUpgrade invalid configuration for Storybook with unstable vite plugin #7953

Closed
1 task done
ulisse1996 opened this issue Nov 9, 2023 · 8 comments · Fixed by #8123
Closed
1 task done

Comments

@ulisse1996
Copy link

ulisse1996 commented Nov 9, 2023

What version of Remix are you using?

2.2.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

  • Create a new remix repository
  • Add storybook
  • Run storybook dev script

Expected Behavior

The storybook dev server should correctly run and integrate with remix vite server and showing onboarding stories stored in stories folder

Actual Behavior

Seems remix plugin is creating an exception causing the following error

Error: server.handleUpgrade() was called more than once with the same socket, possibly due to a misconfiguration at WebSocketServer.completeUpgrade (file:///Users/donato.leone/projects/rox/homelink/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:61573:13) at WebSocketServer.handleUpgrade (file:///Users/donato.leone/projects/rox/homelink/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:61549:10) at Server.hmrServerWsListener (file:///Users/donato.leone/projects/rox/homelink/node_modules/vite/dist/node/chunks/dep-bb8a8339.js:61788:21) at Server.emit (node:events:526:35) at onParserExecuteCommon (node:_http_server:942:14) at onParserExecute (node:_http_server:836:3)

Seems a problem with vite plugin because changing viteFinal in main.ts excluding remix plugins, storybook works as expected

import type { PluginOption } from 'vite';

function isRemix(el: PluginOption) {
  if (!el) {
    return false;
  }
  if (`name` in el) {
    return el.name.includes(`remix`);
  }
  if (Array.isArray(el)) {
    for (const a of el) {
      if (isRemix(a)) {
        return true;
      }
    }
  }

  return false;
}

const config: StorybookConfig = {
  stories: [
    '../stories/**/*.mdx',
    '../stories/**/*.stories.@(js|jsx|mjs|ts|tsx)',
  ],
  addons: [
    '@storybook/addon-links',
    '@storybook/addon-essentials',
    '@storybook/addon-onboarding',
    '@storybook/addon-interactions',
  ],
  framework: {
    name: '@storybook/react-vite',
    options: {},
  },
  docs: {
    autodocs: 'tag',
  },
  viteFinal: (c) => {
    return {
      ...c,
      plugins: (c.plugins || []).filter((el) => {
        return !isRemix(el);
      }),
    };
  },
};
export default config;
`
@hi-ogawa
Copy link
Contributor

Thanks for reporting the issue.
May I ask what kind of specific features you're expecting by saying this?

The storybook dev server should correctly run and integrate with remix vite server ...

I'm not so familiar with recent storybook ecosystem, but from the quick look of their vite/react integration, you might not need "remix" plugin integrated into storybook (though I still don't know what it means by "integrating" exactly).

If that's the case, then similar to what I suggested to workaround when using vitest #7918 (comment), one quicker-and-dirtier approach to filter out "remix" plugin when running storybook could be something like:

// vite.config.ts
import { unstable_vitePlugin as remix } from "@remix-run/dev";
import { defineConfig } from "vite";
import tsconfigPaths from "vite-tsconfig-paths";

export default defineConfig({
  plugins: [
    // turn it off based on executed js entry file
    !process.argv[1].endsWith("storybook/index.js") && remix(),
    tsconfigPaths()
  ],
});

I made a reproduction here https://github.com/hi-ogawa/test-remix-vite-storybook/blob/165e95f8de119a666f959f26673ce59179938540/vite.config.ts#L7 and this trick seems to work on my PC (linux) at least.


I picked up some potentially relevant code if remix team wants to investigate further:

@ulisse1996
Copy link
Author

Hi @hi-ogawa , by

The storybook dev server should correctly run and integrate with remix vite server ...

I mean that as you mention, storybook should just work with their react plugin without any problem even if remix plugin is provided in vite configuration.

The trick seems not working with index.js but just checking if the arg include storybook, seems ok on my side

// vite.config.ts
import { unstable_vitePlugin as remix } from '@remix-run/dev';
import { defineConfig } from 'vite';
import tsconfigPaths from 'vite-tsconfig-paths';

export default defineConfig({
  plugins: [
    !process.argv[1]?.includes('storybook') && remix(),
    tsconfigPaths(),
  ],
  esbuild: {
    jsx: 'automatic',
  },
});

@hi-ogawa
Copy link
Contributor

Thanks for confirming it, also nicer workaround!

I was wondering what svelte ecosystem would do and I found that it looks like storybook "supports" svelte/sveltekit by providing dedicated @storybook/sveltekit separately from @storybook/svelte-vite and, for sveltekit version, they indeed had to remove some incompatible plugins:

https://github.com/storybookjs/storybook/blob/176017d03224f8d0b4add227ebf29a3705f994f5/code/frameworks/sveltekit/src/preset.ts#L23-L28

// disable specific plugins that are not compatible with Storybook
  plugins = (
    await withoutVitePlugins(plugins, [
      'vite-plugin-sveltekit-compile',
      'vite-plugin-sveltekit-guard',
    ])
  ).concat(configOverrides());

So, if remix would follow this "trend", then probably it's unlikely that remix is going to deal with storybook integration by itself, but some sort of workaround might be better fit on storybook side.
Nonetheless, it might still make sense to put some caveat on remix documentation.

@Mordred
Copy link

Mordred commented Nov 21, 2023

I'm also receiving this error. I'm trying to integrate vite with remix plugin to existing express app where I already have a websocket server:

vite.createServer({
    appType: 'custom',
    server: {
      middlewareMode: true,
      hmr: {
        path: '/hmr',
        server: httpServer,
      },
    },
  })

Because remix plugin is creating it's own child vite server: https://github.com/remix-run/remix/blob/main/packages/remix-dev/vite/plugin.ts#L596 http server instance is passed to this child.

Then vite registers two 'upgrade' handlers to the same server https://github.com/vitejs/vite/blob/f576d98265f7db6f09218aeab024ccdc1a82d467/packages/vite/src/node/server/ws.ts#L136

Later when the websocket is connected wss.handleUpgrade is called twice.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Nov 22, 2023

@Mordred Thanks, nice find!

Currently child compiler is used only for transform api to extract module exports,

let [id, code] = await Promise.all([
resolveId(),
fse.readFile(routePath, "utf-8"),
// pluginContainer.transform(...) fails if we don't do this first:
moduleGraph.ensureEntryFromUrl(url, ssr),
]);
let transformed = await pluginContainer.transform(code, id, { ssr });
let [, exports] = esModuleLexer(transformed.code);
let exportNames = exports.map((e) => e.n);
return exportNames;

so I think child compiler doesn't need any of websocket/hmr feature, but it seems vite.createServer will still try to instantiate server (but not listen?) for a different purpose (vitejs/vite#10840 (comment), vitejs/vite#6315).

In this case, however, I think it would work fine if remix simply pass server: { middlewareMode: false } (or just empty server: {}), so that child compiler server doesn't try to attach on("upgrade", ...) for custom server and also websocket server won't start listen automatically since middlewareMode: false.

https://github.com/vitejs/vite/blob/f576d98265f7db6f09218aeab024ccdc1a82d467/packages/vite/src/node/server/index.ts#L789-L808

@Mordred
Copy link

Mordred commented Nov 29, 2023

@markdalgleish #8123 is only adding some documentation, but this bug is not fixed. Can you reopen this issue until #8095 is solved?

@hi-ogawa
Copy link
Contributor

@Mordred I think your issue with server.hmr.server vite config is fixed by #8108, which is probably available in nightly release already. Would you confirm on your side?

@Mordred
Copy link

Mordred commented Nov 29, 2023

Thanks .. you are right. I was testing v2.3.1 .. Good job

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

Successfully merging a pull request may close this issue.

4 participants