-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Addon-actions: Fix ESM by upgrading from uuid-browser to uuid #22037
Addon-actions: Fix ESM by upgrading from uuid-browser to uuid #22037
Conversation
The library uuid-browser doesn't support esm, so the start with webpack fails.
cebe3b1
to
59c72ce
Compare
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Pull request alert summary
📊 Modified Dependency Overview:
|
@@ -117,7 +117,6 @@ const INCLUDE_CANDIDATES = [ | |||
'ts-dedent', | |||
'unfetch', | |||
'util-deprecate', | |||
'uuid-browser/v4', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if uuid
package also needs to be here? It seems that this list has been updated in the past to avoid some issues:
- Fix: vite dev and yarn pnp + vite by unreverting and adding lodash files to optimizeDeps.include #21535
- [Bug]: Your application tried to access @storybook/builder-vite, but it isn't declared in your dependencies v7.0.0-beta.59 #21548
The pull request that created this list in the first place is this one:
That one says that the vite builder was able to be tested with:
yarn task --task smoke-test --template react-vite/default-js --force
yarn task --task smoke-test --template vue3-vite/default-js --force
Maybe worth trying to run that here unless the same tests are also being ran in CI automatically?
The comment where this list gets used is this one:
storybook/code/lib/builder-vite/src/optimizeDeps.ts
Lines 146 to 148 in 59c72ce
// We need Vite to precompile these dependencies, because they contain non-ESM code that would break | |
// if we served it directly to the browser. | |
include: [...include, ...(config.optimizeDeps?.include || [])], |
So it could be that uuid
is indeed not necessary to add here if it already ships ESM code?
I'm not that familiar with this code but these are the only worries I would have about these code changes.
I've tested using the uuid
package in place of uuid-browser/v4
successfully in our own project where we have manually set a webpack alias for uuid-browser/v4
import to point to this file instead:
// This file acts like a monkeypatch for fixing this issue:
// https://github.com/storybookjs/storybook/issues/21916
//
// We have custom webpack configuration that makes Storybook load
// this file instead of the file from `uuid-browser` package so that
// we can get the imports to work correctly.
//
// Once the issue mentioned above is fixed, this file should be removed.
import { v4 as uuidv4 } from 'uuid';
export default uuidv4;
This workaround has worked for us and usage of uuid
package in place of uuid-browser
succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran a storybook locally on this branch, and vite didn't warn about this...
Addon-actions: Fix ESM by upgrading from uuid-browser to uuid
Closes #21916
What I did
The library uuid-browser doesn't support esm, so running storybook with webpack could produce an error
Can't resolve 'uuid-browser/v4
How to test
Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]