Vite: Replace vite-plugin-externals with custom plugin #20698

Dschungelabenteuer · 2023-01-19T23:32:14Z

Issue: #20643

What I did

When used in dev mode, vite-plugin-externals breaks HMR for vite-based projects.
When disabled in dev mode, it completely prevents Storybook from working on vite + pnpm (and probably Yarn PnP) projects.
I replaced that plugin with a local one, which is straightforward and quite naive

What I'm not sure I checked correctly

Production build works fine
That the [Bug]: Investigate code getting pre-bundled multiple times #20514 issue about code duplication is still fixed (I've searched for

What I'm sure I didn't checked

Yarn PnP

How to test

You can create any Vite sandbox, but it will still require you to change the package manager to pnpm so that dependencies do not get hoisted.
Make sure you get the following error: [vite] Internal server error: Failed to resolve import "@storybook/preview-api" from "../../../../../../virtual:/@storybook/builder-vite/vite-app.js".
Build the @storybook/builder-vite package from the source branch of this PR
Copy the generated dist output of @storybook/builder-vite to replace the one in your pnpm workspace
Start storybook
Be happy because the error is gone!

Checklist

Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
Make sure to add/update documentation regarding your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Maintainers

If this PR should be tested against many or all sandboxes,
make sure to add the ci:merged or ci:daily GH label to it.
Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

…rnals

IanVS · 2023-01-23T04:11:44Z

~~This is working fine for me locally, but is failing in CI. @ndelangen maybe you can take a look?~~ Figured it out.

I've confirmed this approach does avoid duplicate imports, and HMR works correctly. I'm a bit nervous that the simple find/replace might be a bit too fragile, but it's building which I think means we're okay. We might consider changing back to vite-plugin-externals if crcong/vite-plugin-externals#27 is merged and we find that this internal plugin causes us problems.

code/lib/builder-vite/src/plugins/externals-plugin.ts

ndelangen

Needs test, needs perf improvements.

I can work on this if needed.

code/lib/builder-vite/src/plugins/externals-plugin.ts

ndelangen · 2023-01-23T10:47:34Z

code/lib/builder-vite/src/plugins/externals-plugin.ts

+      imports.forEach(({ n: path, ss: startPosition, se: endPosition }) => {
+        const packageName = path as SingleGlobalName | undefined;
+        const packageAndDelimiters = new RegExp(`.${packageName}.`);
+        if (packageName && globalsList.includes(packageName)) {
+          src.update(
+            startPosition,
+            endPosition,
+            src
+              .slice(startPosition, endPosition)
+              .replace('import ', 'const ')
+              .replace('import{', 'const {')
+              .replaceAll(' as ', ': ')
+              .replace(' from ', ' = ')
+              .replace('}from', '} = ')
+              .replace(packageAndDelimiters, globals[packageName])
+          );
+        }
+      });


I am scared, without a proper test-suite. ... we have tests for this?

We should extract this code into a function and test the flip out of it.

I would also recommend writing a single regex + a single replace, that's easier to grok than 6 separate replace commands.?

IanVS · 2023-01-23T12:55:42Z

Thanks, I completely agree. Just wanted to prove out that the approach would work first, and it seems like it will. If you have time to work on it that would be great, otherwise I'll try to take a shot at it tonight. Or maybe @Dschungelabenteuer would like to?

Dschungelabenteuer · 2023-01-23T20:27:41Z

@IanVS I can try while you take some rest until you feel better :) Eventually I'll probably need some guidance around tests, though!

stevezhu · 2023-01-23T22:15:10Z

I can also help on anything here if needed.

…d basic tests

stevezhu · 2023-01-24T04:33:10Z

Don't mean to derail the efforts here, but it's also possible to instead more simply wrap the original plugin within a custom plugin, allowing bug fixes, swapping out certain parts, or extending, etc. Could be easier to go with this than to maintain a new custom plugin.

import type { Plugin } from 'vite';
import { globals } from '@storybook/preview/globals';

import { viteExternalsPlugin } from 'vite-plugin-externals';

export async function externalsPlugin() {
  const plugin = viteExternalsPlugin(globals, { useWindow: false });
  const config = plugin.config as Extract<
    ReturnType<typeof viteExternalsPlugin>['config'],
    (...args: any[]) => any
  >;
  return {
    ...plugin,
    name: 'storybook:externals-plugin',
    async config(...configArgs) {
      const conf = await config(...configArgs);
      return {
        resolve: {
          alias: conf?.resolve?.alias,
        },
      };
    },
  } satisfies Plugin;
}

Note on the following part:

Extract<
  ReturnType<typeof viteExternalsPlugin>['config'],
  (...args: any[]) => any
>

We have to narrow the type here to the function portion of the union since the original plugin returns Plugin explicitly instead of using satisfies.

ndelangen · 2023-01-24T12:24:16Z

@Dschungelabenteuer Do you want @stevezhu to take over?

I think, with a bit of cleanup this PR is good to go.
And this has high priority for me to get merged.

Dschungelabenteuer · 2023-01-24T13:42:55Z

@Dschungelabenteuer Do you want @stevezhu to take over?

I think, with a bit of cleanup this PR is good to go. And this has high priority for me to get merged.

I'm still at work so if @stevezhu wants to give it a shot, please feel free :)

ndelangen · 2023-01-24T13:48:19Z

High priority means, "preferably this week" 😄

IanVS · 2023-01-24T15:07:08Z

@stevezhu It might be interesting to compare the performance of the two approaches, is that something that you would be interested in trying out, to see if there's much of a difference?

stevezhu · 2023-01-24T16:30:46Z

PR for the suggestion here: #20770

@IanVS Simple performance test in one of the comments #20770 (comment)

IanVS · 2023-01-24T20:51:28Z

Thanks @stevezhu. I wanted to get a bit more granular perf numbers for just the plugin, so I pushed up two branches, https://github.com/storybookjs/storybook/tree/timing/external-vite-plugin and https://github.com/storybookjs/storybook/tree/timing/internal-vite-plugin. I added timings to the config and transform hooks separately, and this is roughly what I found in the react-ts sandbox:

external (wrapped dependency):

config: 0.05 sec
transform: 0.65 sec

internal:

config: 4 sec
transform: 0.02 sec

This surprised me, since the config hooks are nearly identical between the two. But, the external plugin was using a newer version of fs-extra, and when I updated storybook, the times matched up. So I submitted #20772 because I think we'll want to update fs-extra no matter what.

At any rate, it does seem like the approach in this PR is slightly more performant, and we should keep in mind that the sandbox is a small project, and we could expect the transform time to grow with the number of files being transformed. There's also something nice about controlling our own destiny and not relying on another maintainer to merge fixes, though the workaround in #20770 is clever and would indeed do what we need.

In summary, I think either approach would be fine, the internal plugin is a bit faster (after the fs-extra upgrade), but if we don't feel comfortable with its robustness we could use the wrapped dependency instead.

stevezhu · 2023-01-24T23:04:52Z

The fs-extra performance was a great find! Ultimately up to you on what you think will be the better solution; just wanted to offer up the other option. Seeing as how the external plugin is relatively unmaintained and with the performance gains, I'm definitely leaning towards having an internal plugin as well.

code/lib/builder-vite/src/plugins/externals-plugin.ts

Dschungelabenteuer · 2023-01-25T11:30:04Z

Nice insight from both of you guys, I was quite curious about the transform hook which seemed like the most relevant concern, especially because it looks like candidate files are being traversed twice (first by es-module-lexer just like in the current PR, then with acorn to replace tokens based on file's AST, which is safer but also heavier. I would never have suspected potential performance issues inside the config hook, nice catch!

Do we have a clear idea of the amount of files served by Vite that are actually transformed by the plugin? I mean, there are the obvious virtual files such as vite-app.js and setup-addons.js, but is there something else we should be paying close attention to? Maybe this could help consolidate our transform process so that we feel more confident about this?

stevezhu

A few small comments

code/lib/builder-vite/src/plugins/externals-plugin.ts

IanVS

I think this is really close, but there are a couple of cases we should handle (and test).

code/lib/builder-vite/src/plugins/externals-plugin.test.ts

code/lib/builder-vite/src/plugins/externals-plugin.ts

… added more tests

IanVS

This looks good to me, I tested it out in a new sandbox and yarn storybook starts fine and HMR still works. I'd love if we can get this merged and released ASAP since it fixes a pretty large issue for vite (and especially pnpm) users. Thanks for all the great work here @Dschungelabenteuer and for the help/reviews/ideas @stevezhu. 🙌 🚀

@ndelangen how's it look to you?

code/lib/builder-vite/src/plugins/externals-plugin.ts

dryton · 2023-01-30T10:43:12Z

I'm seeing an issue in relation to this change in beta.36:

11:03:43 AM [vite] Internal server error: src.update is not a function
  Plugin: storybook:external-globals-plugin
  File: /virtual:/@storybook/builder-vite/vite-app.js
      at null.<anonymous> (/Users/name/project/node_modules/@storybook/builder-vite/dist/index.js:162:4407)
      at Array.forEach (<anonymous>)
      at TransformContext.transform (/Users/name/project/node_modules/@storybook/builder-vite/dist/index.js:162:4148)
      at Object.transform (file:///Users/name/project/node_modules/vite/dist/node/chunks/dep-665b0112.js:41031:44)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at async loadAndTransform (file:///Users/name/project/node_modules/vite/dist/node/chunks/dep-665b0112.js:37292:29)
11:03:45 AM [vite] error while updating dependencies:
Error: ENOENT: no such file or directory, rename '/Users/name/project/node_modules/.cache/.vite-storybook/deps_temp' -> '/Users/name/project/node_modules/.cache/.vite-storybook/deps'
    at renameSync (node:fs:980:3)
    at Object.commit (file:///Users/name/project/node_modules/vite/dist/node/chunks/dep-665b0112.js:42219:19)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async commitProcessing (file:///Users/name/project/node_modules/vite/dist/node/chunks/dep-665b0112.js:41694:17)
    at async runOptimizer (file:///Users/name/project/node_modules/vite/dist/node/chunks/dep-665b0112.js:41732:17)

IanVS · 2023-01-30T11:37:46Z

@dryton it looks like you might be hitting vitejs/vite#9986 or some variation on it. Are you able to reproduce it reliably, or does it only happen occasionally? If you can reproduce it, can you please open an issue with a link to a reproduction?

Dschungelabenteuer · 2023-01-30T17:21:09Z

@dryton as mentioned in this discussion, I think we'll have to upgrade the minimum expected magic-string version, since the update method only exists since 0.26.6 and the @storybook/builder-vite only sets it to ^0.26.1.

Following #20698, [some users reported an error thrown around Magic String's `update` method](#20698 (comment)). ## What I did It turns out the `update` method was introduced in [version `0.26.6`](https://github.com/Rich-Harris/magic-string/blob/master/CHANGELOG.md#0266-2022-10-05), but Storybook packages [actually set it to `^0.26.1`](https://github.com/storybookjs/storybook/blob/next/code/lib/builder-vite/package.json#L62). I've opted for 0.27.0 instead of 0.26.6 because it does not seem to have any breaking changes and we could benefit from performance improvements they made. ## How to test This should be pretty straight-forward. I think the issue does not occur on clean installs because it should already pick `magic-string`'s latest patch. Let's see if CI passes! ## Checklist  - [ ] Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests) - [ ] Make sure to add/update documentation regarding your changes - [ ] If you are deprecating/removing a feature, make sure to update [MIGRATION.MD](https://github.com/storybookjs/storybook/blob/next/MIGRATION.md) #### Maintainers - [ ] If this PR should be tested against many or all sandboxes, make sure to add the `ci:merged` or `ci:daily` GH label to it. - [X] Make sure this PR contains **one** of the labels below. `["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]`

Naive implementation of local vite plugin to replace vite-plugin-exte…

9642d04

…rnals

stevezhu mentioned this pull request Jan 20, 2023

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

Closed

Dschungelabenteuer and others added 3 commits January 21, 2023 18:52

Merge branch 'next' into fix-hmr-vite

04a76cf

Remove unused imports

b49050a

Remove vite-plugin-externals

61acc22

IanVS added bug builder-vite labels Jan 23, 2023

IanVS added 4 commits January 22, 2023 20:37

Replace all as in import specifiers with colons

69f23e4

Return sourcemap

22af7fe

Create aliases for dev

b78244e

Add a few docblocks

1561785

IanVS marked this pull request as ready for review January 23, 2023 04:24

Handle minified code

563fa6c

ndelangen reviewed Jan 23, 2023

View reviewed changes

code/lib/builder-vite/src/plugins/externals-plugin.ts Outdated Show resolved Hide resolved

ndelangen reviewed Jan 23, 2023

View reviewed changes

ndelangen self-assigned this Jan 23, 2023

Merge branch 'storybookjs:next' into fix-hmr-vite

b72ca3e

Parallel cache creation, grouped all replaces as a single regex, adde…

76ca07d

…d basic tests

stevezhu mentioned this pull request Jan 24, 2023

fix: Wrap vite-plugin-external in custom plugin and fix issues #20770

Closed

5 tasks

stevezhu reviewed Jan 24, 2023

View reviewed changes

code/lib/builder-vite/src/plugins/externals-plugin.ts Outdated Show resolved Hide resolved

IanVS mentioned this pull request Jan 25, 2023

fails to install on ubuntu, pnpm@latest, typescript@beta, react@18.1, webpack@latest ? #18111

Closed

stevezhu reviewed Jan 25, 2023

View reviewed changes

Dschungelabenteuer and others added 3 commits January 25, 2023 18:45

Merge branch 'next' into fix-hmr-vite

007b5bb

Cleaned up pointless types

71c3cc2

Merge remote-tracking branch 'origin/next' into fix-hmr-vite

ab1508e

IanVS changed the title ~~builder-vite: use local plugin instead of vite-plugin-externals~~ Vite: Replace vite-plugin-externals with custom plugin Jan 25, 2023

IanVS requested changes Jan 26, 2023

View reviewed changes

Dschungelabenteuer added 2 commits January 26, 2023 14:22

Fixed default imports, added support for import namespace specifiers,…

f86ff03

… added more tests

Pass globals definition through plugin's arguments

e9592b2

Dschungelabenteuer requested a review from IanVS January 26, 2023 15:34

IanVS approved these changes Jan 27, 2023

View reviewed changes

stevezhu reviewed Jan 27, 2023

View reviewed changes

code/lib/builder-vite/src/plugins/externals-plugin.ts Outdated Show resolved Hide resolved

Dschungelabenteuer added 2 commits January 27, 2023 17:12

Made names more consistent

759047e

Merge remote-tracking branch 'origin/next' into fix-hmr-vite

a602c6a

ndelangen merged commit 95f4adc into storybookjs:next Jan 27, 2023

Dschungelabenteuer mentioned this pull request Jan 30, 2023

Deps: Upgrade magic-string to 0.27.0 #20839

Merged

5 tasks

rbardini mentioned this pull request Mar 1, 2023

[Bug]: Preview broken in v7 dev mode with web-components-vite - module is undefined #21334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vite: Replace vite-plugin-externals with custom plugin #20698

Vite: Replace vite-plugin-externals with custom plugin #20698

Dschungelabenteuer commented Jan 19, 2023 •

edited by IanVS

Loading

IanVS commented Jan 23, 2023 •

edited

Loading

ndelangen left a comment

ndelangen Jan 23, 2023

IanVS commented Jan 23, 2023

Dschungelabenteuer commented Jan 23, 2023

stevezhu commented Jan 23, 2023

stevezhu commented Jan 24, 2023 •

edited

Loading

ndelangen commented Jan 24, 2023

Dschungelabenteuer commented Jan 24, 2023

ndelangen commented Jan 24, 2023

IanVS commented Jan 24, 2023

stevezhu commented Jan 24, 2023 •

edited

Loading

IanVS commented Jan 24, 2023

stevezhu commented Jan 24, 2023 •

edited

Loading

Dschungelabenteuer commented Jan 25, 2023

stevezhu left a comment

IanVS left a comment

IanVS left a comment

dryton commented Jan 30, 2023

IanVS commented Jan 30, 2023

Dschungelabenteuer commented Jan 30, 2023

Vite: Replace vite-plugin-externals with custom plugin #20698

Vite: Replace vite-plugin-externals with custom plugin #20698

Conversation

Dschungelabenteuer commented Jan 19, 2023 • edited by IanVS Loading

What I did

What I'm not sure I checked correctly

What I'm sure I didn't checked

How to test

Checklist

Maintainers

IanVS commented Jan 23, 2023 • edited Loading

ndelangen left a comment

Choose a reason for hiding this comment

ndelangen Jan 23, 2023

Choose a reason for hiding this comment

IanVS commented Jan 23, 2023

Dschungelabenteuer commented Jan 23, 2023

stevezhu commented Jan 23, 2023

stevezhu commented Jan 24, 2023 • edited Loading

ndelangen commented Jan 24, 2023

Dschungelabenteuer commented Jan 24, 2023

ndelangen commented Jan 24, 2023

IanVS commented Jan 24, 2023

stevezhu commented Jan 24, 2023 • edited Loading

IanVS commented Jan 24, 2023

stevezhu commented Jan 24, 2023 • edited Loading

Dschungelabenteuer commented Jan 25, 2023

stevezhu left a comment

Choose a reason for hiding this comment

IanVS left a comment

Choose a reason for hiding this comment

IanVS left a comment

Choose a reason for hiding this comment

dryton commented Jan 30, 2023

IanVS commented Jan 30, 2023

Dschungelabenteuer commented Jan 30, 2023

Dschungelabenteuer commented Jan 19, 2023 •

edited by IanVS

Loading

IanVS commented Jan 23, 2023 •

edited

Loading

stevezhu commented Jan 24, 2023 •

edited

Loading

stevezhu commented Jan 24, 2023 •

edited

Loading

stevezhu commented Jan 24, 2023 •

edited

Loading