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

Fix React HMR in story files #53

Merged
merged 1 commit into from
Jun 28, 2021
Merged

Fix React HMR in story files #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/storybook-builder-vite/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"dependencies": {
"@mdx-js/mdx": "^1.6.22",
"@storybook/csf-tools": "^6.3.0-rc.4",
"@vitejs/plugin-react-refresh": "^1.3.2",
"@vitejs/plugin-react-refresh": "^1.3.4",
"glob-promise": "^4.1.0",
"vite-plugin-mdx": "^3.5.1"
},
Expand Down
5 changes: 4 additions & 1 deletion packages/storybook-builder-vite/vite-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ module.exports.pluginConfig = function pluginConfig(options, type) {

if (type === 'development') {
if (framework === 'react') {
plugins.push(require('@vitejs/plugin-react-refresh')());
plugins.push(require('@vitejs/plugin-react-refresh')({
// Do not treat story files as HMR boundaries, storybook itself needs to handle them.
exclude: [/\.stories\.(t|j)sx?$/, /node_modules/],
Copy link

Choose a reason for hiding this comment

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

Probably, also excluding node_modules would lead to bugs with linked dependencies which HMR should be managed by vite

Copy link

Choose a reason for hiding this comment

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

There should be also ability to override the exclude, as not everyone names files as stories

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, also excluding node_modules would lead to bugs with linked dependencies which HMR should be managed by vite

Excluding node_modules is the default behavior of plugin-react-refresh, so I wanted to keep that behavior for now, until/unless we hear of problems.

There should be also ability to override the exclude, as not everyone names files as stories

There is, it is possible to change any of this config through the use of viteFinal in the storybook configuration file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's also possible to read the list of stories (or the list of file globs) from the config variable, I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately it looks like stories from the config is not exposed to the builder. I logged out the options and didn't see it, and it's not in the type definitions, from what I can tell: https://github.com/storybookjs/storybook/blob/next/lib/core-common/src/types.ts#L164

Copy link
Collaborator

@eirslett eirslett Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@shilman should that config option maybe be exposed in the builder API?

One problem is that if Vite is using another glob library than Storybook, there could be a mismatch between the two even if the glob pattern is the same. So neither will be a fool-proof solution.

Copy link
Member

@shilman shilman Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you can get it from options.presets.apply('stories') but I didn't test @IanVS @eirslett

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking into this, it's true that we can get the stories as @shilman suggested. However, those are micromatch globs, and @vitejs/react-fast-refresh uses picomatch, which does not allow brace expansion. We would need to resolve the entire list of files and pass those to the plugin. Since this whole approach is a bit of a bandaid, I propose that we hold off on any more complicated exclusion logic until/unless we hear of problems. Maybe we'll find a better solution entirely that works for more than just the react framework.

}));
}
}

Expand Down
66 changes: 55 additions & 11 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ __metadata:
languageName: node
linkType: hard

"@babel/core@npm:7.14.5, @babel/core@npm:^7.1.0, @babel/core@npm:^7.1.6, @babel/core@npm:^7.12.10, @babel/core@npm:^7.12.13, @babel/core@npm:^7.7.5":
"@babel/core@npm:7.14.5, @babel/core@npm:^7.1.0, @babel/core@npm:^7.1.6, @babel/core@npm:^7.12.10, @babel/core@npm:^7.7.5":
version: 7.14.5
resolution: "@babel/core@npm:7.14.5"
dependencies:
Expand All @@ -84,6 +84,29 @@ __metadata:
languageName: node
linkType: hard

"@babel/core@npm:7.14.6, @babel/core@npm:^7.14.6":
version: 7.14.6
resolution: "@babel/core@npm:7.14.6"
dependencies:
"@babel/code-frame": ^7.14.5
"@babel/generator": ^7.14.5
"@babel/helper-compilation-targets": ^7.14.5
"@babel/helper-module-transforms": ^7.14.5
"@babel/helpers": ^7.14.6
"@babel/parser": ^7.14.6
"@babel/template": ^7.14.5
"@babel/traverse": ^7.14.5
"@babel/types": ^7.14.5
convert-source-map: ^1.7.0
debug: ^4.1.0
gensync: ^1.0.0-beta.2
json5: ^2.1.2
semver: ^6.3.0
source-map: ^0.5.0
checksum: 239c4892d54f1d6e3a9a3972a7579138da6ff5308b9c08e4c80c9cd09282b6a921f58338851675fdb80b1cf9dd14f4176674917b97aa430bf1d50c0052bbb13a
languageName: node
linkType: hard

"@babel/generator@npm:^7.12.11, @babel/generator@npm:^7.12.5, @babel/generator@npm:^7.14.5":
version: 7.14.5
resolution: "@babel/generator@npm:7.14.5"
Expand Down Expand Up @@ -374,6 +397,17 @@ __metadata:
languageName: node
linkType: hard

"@babel/helpers@npm:^7.14.6":
version: 7.14.6
resolution: "@babel/helpers@npm:7.14.6"
dependencies:
"@babel/template": ^7.14.5
"@babel/traverse": ^7.14.5
"@babel/types": ^7.14.5
checksum: c5c3bd0f9618cdb8895d89171fe0b89c0b119bf8c9f96aff869d95b9208628172a882a132f8f76a218e0e68d8c3316f65b60af9b3a3a778d9b9adce004ca52c7
languageName: node
linkType: hard

"@babel/highlight@npm:^7.10.4, @babel/highlight@npm:^7.14.5":
version: 7.14.5
resolution: "@babel/highlight@npm:7.14.5"
Expand All @@ -394,6 +428,15 @@ __metadata:
languageName: node
linkType: hard

"@babel/parser@npm:^7.14.6":
version: 7.14.7
resolution: "@babel/parser@npm:7.14.7"
bin:
parser: ./bin/babel-parser.js
checksum: 8c7c7fd9735d9de2a0531fe54338d76972aeb9a414902f5edfc317c421a2da6a8772d04dce5444c89065ae2719c48433153f9cbaa14754b1bf8d6154915095ff
languageName: node
linkType: hard

"@babel/plugin-bugfix-v8-spread-parameters-in-optional-chaining@npm:^7.14.5":
version: 7.14.5
resolution: "@babel/plugin-bugfix-v8-spread-parameters-in-optional-chaining@npm:7.14.5"
Expand Down Expand Up @@ -1162,7 +1205,7 @@ __metadata:
languageName: node
linkType: hard

"@babel/plugin-transform-react-jsx-self@npm:^7.12.13":
"@babel/plugin-transform-react-jsx-self@npm:^7.14.5":
version: 7.14.5
resolution: "@babel/plugin-transform-react-jsx-self@npm:7.14.5"
dependencies:
Expand All @@ -1173,7 +1216,7 @@ __metadata:
languageName: node
linkType: hard

"@babel/plugin-transform-react-jsx-source@npm:^7.12.13":
"@babel/plugin-transform-react-jsx-source@npm:^7.14.5":
version: 7.14.5
resolution: "@babel/plugin-transform-react-jsx-source@npm:7.14.5"
dependencies:
Expand Down Expand Up @@ -3552,15 +3595,16 @@ __metadata:
languageName: node
linkType: hard

"@vitejs/plugin-react-refresh@npm:^1.3.2":
version: 1.3.3
resolution: "@vitejs/plugin-react-refresh@npm:1.3.3"
"@vitejs/plugin-react-refresh@npm:^1.3.4":
version: 1.3.4
resolution: "@vitejs/plugin-react-refresh@npm:1.3.4"
dependencies:
"@babel/core": ^7.12.13
"@babel/plugin-transform-react-jsx-self": ^7.12.13
"@babel/plugin-transform-react-jsx-source": ^7.12.13
"@babel/core": ^7.14.6
"@babel/plugin-transform-react-jsx-self": ^7.14.5
"@babel/plugin-transform-react-jsx-source": ^7.14.5
"@rollup/pluginutils": ^4.1.0
react-refresh: ^0.9.0
checksum: f8c579546c24e7334abb63bd6576508e755f7087fd21df19679784824ac3154515e2c6a900eb9e599051ec0d4f1f1d2c02afcabf6d8fea10c58a2d8451a77467
checksum: 86823a114d0563376dadb75d23da581b90c23a92bbce11bd1c97d0ea4764d52f6cd43e6d7a58a6116a0558bc32492499dac974c16109cee16f50c4af0a3c61cc
languageName: node
linkType: hard

Expand Down Expand Up @@ -12916,7 +12960,7 @@ fsevents@^1.2.7:
"@storybook/addon-svelte-csf": ^1.1.0
"@storybook/csf-tools": ^6.3.0-rc.4
"@sveltejs/vite-plugin-svelte": ^1.0.0-next.7
"@vitejs/plugin-react-refresh": ^1.3.2
"@vitejs/plugin-react-refresh": ^1.3.4
"@vitejs/plugin-vue": ^1.2.1
glob-promise: ^4.1.0
vite-plugin-mdx: ^3.5.1
Expand Down