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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 20, 2023
Merged

Addon-actions: Fix ESM by upgrading from uuid-browser to uuid #22037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59c72ce
Fix #21916 - using uuid library instead of uuid-browser
Apr 11, 2023
5e26657
Merge branch 'next' into fix/21916-uuid-browser-not-found
ndelangen Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion code/addons/actions/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,11 @@
"react-inspector": "^6.0.0",
"telejson": "^7.0.3",
"ts-dedent": "^2.0.0",
"uuid-browser": "^3.1.0"
"uuid": "^9.0.0"
},
"devDependencies": {
"@types/lodash": "^4.14.167",
"@types/uuid": "^9.0.1",
"typescript": "~4.9.3"
},
"peerDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion code/addons/actions/src/runtime/action.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import uuidv4 from 'uuid-browser/v4';
import { v4 as uuidv4 } from 'uuid';
import { addons } from '@storybook/preview-api';
import { EVENT_ID } from '../constants';
import type { ActionDisplay, ActionOptions, HandlerFunction } from '../models';
Expand Down
2 changes: 0 additions & 2 deletions code/addons/actions/src/typings.d.ts
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion code/lib/builder-vite/src/optimizeDeps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ const INCLUDE_CANDIDATES = [
'ts-dedent',
'unfetch',
'util-deprecate',
'uuid-browser/v4',
Copy link
Contributor

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:

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.

Copy link
Member

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...

'vue',
'warning',
];
Expand Down
26 changes: 18 additions & 8 deletions code/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5017,6 +5017,7 @@ __metadata:
"@storybook/theming": 7.1.0-alpha.7
"@storybook/types": 7.1.0-alpha.7
"@types/lodash": ^4.14.167
"@types/uuid": ^9.0.1
dequal: ^2.0.2
lodash: ^4.17.21
polished: ^4.2.2
Expand All @@ -5025,7 +5026,7 @@ __metadata:
telejson: ^7.0.3
ts-dedent: ^2.0.0
typescript: ~4.9.3
uuid-browser: ^3.1.0
uuid: ^9.0.0
peerDependencies:
react: ^16.8.0 || ^17.0.0 || ^18.0.0
react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0
Expand Down Expand Up @@ -8808,6 +8809,13 @@ __metadata:
languageName: node
linkType: hard

"@types/uuid@npm:^9.0.1":
version: 9.0.1
resolution: "@types/uuid@npm:9.0.1"
checksum: 234e14e053504a98532bb5d1490c8d649fe24ae04a94ba042b09b380a900094df1032aa7c3864b74b7a85a0a5e97530b2d1340048ed9d07855140cb99b2cefc8
languageName: node
linkType: hard

"@types/webpack-dev-middleware@npm:^5.3.0":
version: 5.3.0
resolution: "@types/webpack-dev-middleware@npm:5.3.0"
Expand Down Expand Up @@ -29716,13 +29724,6 @@ __metadata:
languageName: node
linkType: hard

"uuid-browser@npm:^3.1.0":
version: 3.1.0
resolution: "uuid-browser@npm:3.1.0"
checksum: bfb6bcc8cc75c1adf776370c4f86d00ee5682f7315c8bccb99938e53dafae189ef6a4dc125e67abd2a2cdfaad6020690fe4cb67dbd5b39f32d3ba75fb713d807
languageName: node
linkType: hard

"uuid@npm:8.3.2, uuid@npm:^8.0.0, uuid@npm:^8.3.2":
version: 8.3.2
resolution: "uuid@npm:8.3.2"
Expand All @@ -29732,6 +29733,15 @@ __metadata:
languageName: node
linkType: hard

"uuid@npm:^9.0.0":
version: 9.0.0
resolution: "uuid@npm:9.0.0"
bin:
uuid: dist/bin/uuid
checksum: 8867e438990d1d33ac61093e2e4e3477a2148b844e4fa9e3c2360fa4399292429c4b6ec64537eb1659c97b2d10db349c673ad58b50e2824a11e0d3630de3c056
languageName: node
linkType: hard

"uvu@npm:^0.5.0":
version: 0.5.6
resolution: "uvu@npm:0.5.6"
Expand Down