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

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

Closed

Conversation

stevezhu
Copy link

@stevezhu stevezhu commented Jan 24, 2023

Issue: #20643

Alternate solution for: #20698

What I did

HMR was broken by vite-plugin-externals due to returning the entire config from the config hook instead of the partial config as written in the vite api docs: https://vitejs.dev/guide/api-plugin.html#vite-specific-hooks.

Recreates the same fix as in crcong/vite-plugin-externals#27 by wrapping the plugin in a custom plugin and overriding the result of the config hook.

How to test

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Build @storybook/builder-vite, yarn build --watch builder-vite from code/ directory
  3. Open Storybook in your browser
  4. Ensure HMR is working
  5. Ensure there is not a regression with [Bug]: Investigate code getting pre-bundled multiple times #20514

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"]

@stevezhu
Copy link
Author

stevezhu commented Jan 24, 2023

Performance test using https://github.com/Shinigami92/vite-plugin-time-reporter

Ran a few times with the sandbox build.

Results seem comparable. I would imagine most of the build time is going towards other parts of of the build process.

This PR (#20770)

(index) time
build 11.124s
render 3.146s
full 20.196s
(index) time
build 14.477s
render 4.086s
full 24.790s
(index) time
build 13.324s
render 3.856s
full 23.133s

#20698

(index) time
build 14.411s
render 5.921s
full 26.623s
(index) time
build 10.516s
render 3.139s
full 19.009s
(index) time
build 11.514s
render 3.339s
full 19.431s

@stevezhu
Copy link
Author

Opted for #20698

@stevezhu stevezhu closed this Jan 27, 2023
@Dschungelabenteuer
Copy link
Member

@stevezhu thanks again for all your constructive feedback, appreciated! 🤗

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.

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