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

Vite: Fix prebundling #19978

Merged
merged 28 commits into from
Nov 28, 2022
Merged

Vite: Fix prebundling #19978

merged 28 commits into from
Nov 28, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Nov 28, 2022

Issue: N/A

@ndelangen noticed that /sb-preview/runtime.mjs was being bundled into the bundle created by vite for the iframe, rather than being externalized and requested from the static output folder as it should have been.

What I did

This replaces the esbuild plugin, @fal-works/esbuild-plugin-global-externals with a rollup plugin that works more similarly to the webpack externals method. We can give it a list of packages which should be treated as globals, and the name of the global variable where they can be accessed.

This also removes the re-exports of storybook packages from the vite frameworks, since they are not accessed from global scope instead. This essentially reverts #19216.

And I simplified @storybook/preview, since we no longer need to generate the ModuleInfo objects for the esbuild plugin. And we can export the mapping between package names and global variables, to use in both vite and webpack.

How to test

CI. We should also check that pnpm still works correctly.

@IanVS IanVS requested a review from ndelangen November 28, 2022 01:50
@IanVS
Copy link
Member Author

IanVS commented Nov 28, 2022

This is mostly working. Building storybook succeeds, and sb-preview/runtime.mjs is no longer being bundled. Storybook loads the page correctly, but changing stories by clicking in the sidebar is broken, in both dev and built storybooks. Refreshing the page will load the correct story. The webpack projects are working correctly, just not vite, and I'm not sure why. Maybe @ndelangen you might have an idea here?

@ndelangen
Copy link
Member

ndelangen commented Nov 28, 2022

@IanVS I'm still seeing this in the dev mode of storybook:

Screenshot 2022-11-28 at 09 00 26

Also see it in build-mode


I build-mode i can see it working though:
Screenshot 2022-11-28 at 09 13 15

Also confirmed to be working in dev-mode:
Screenshot 2022-11-28 at 09 15 43

@ndelangen ndelangen self-assigned this Nov 28, 2022
@ndelangen
Copy link
Member

ndelangen commented Nov 28, 2022

The CI error is weird because it's a peerDep and we add it to the project when initializing storybook:

How could the dep not be there?

@ndelangen
Copy link
Member

can confirm, it's not being added as a dep when running the sanbox command:

  "dependencies": {
    "@types/node": "16",
    "lit": "^2.4.0"
  },

Need to debug this, why

@ndelangen
Copy link
Member

ndelangen commented Nov 28, 2022

@IanVS I think I found the problem:

rollupOptions: {
// Do not try to bundle the storybook runtime, it is copied into the output dir after the build.
external: ['/sb-preview/runtime.mjs'],
},

conflicts with:

// sandbox/lit-vite-default-ts/vite.config.ts

import { defineConfig } from 'vite';

export default defineConfig({
  build: {
    lib: {
      entry: 'src/my-element.ts',
      formats: ['es']
    },
    rollupOptions: {
      external: /^lit/
    }
  }
})

Might not be getting merged correctly?

Also there's this code:

config.build.rollupOptions = {
...config.build.rollupOptions,
input: iframePath,
};


Or maybe this:

external: /^lit/

Matches too much?

I tried removing that bit of code from the sandbox's vite.config.ts, and that made no difference..


Another bit of information @IanVS the dev-mode is working fine, it's only a problem when building storybook.

@ndelangen
Copy link
Member

the main.js viteFinal method is the same across all 4 vite sandboxes are exactly the same.

emptyOutDir: false, // do not clean before running Vite build - Storybook has already added assets in there!
sourcemap: true,
};
config.build = mergeConfig(config, {
Copy link
Member Author

Choose a reason for hiding this comment

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

We could remove the mergeConfig here, since we do not want to merge the build config with the user's build config.

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.

2 participants