Append browserHash to absolute config imports #305
Conversation
This reverts commit 0f14b29. # Conflicts: # packages/builder-vite/codegen-iframe-script.ts
These all have esm exports (@storybook/csf is cjs only)
|
To be clear, this is blocked by imports of |
This may be a bug in Vite. Could you point me to a way to reproduce it? |
|
A suggestion for a repro @IanVS:
What we saw was that when internal imports crossed package boundaries, the hash was appended by vite. So I would expect Then make |
|
@patak-dev, here is as minimal repro as I can make. https://github.com/IanVS/vite-fs-import-prebundle-repro Perhaps this is just expected behavior, and I need to find a different way to resolve a file on filesystem into a node_module module that might have been pre-bundled? |
|
@IanVS thanks for the reproduction. I don't think that direct access using I think this is needed:
Even if find a workaround for using direct |
|
Thanks @patak-dev. Do you know what the docs are referring to here? https://vitejs.dev/config/#server-fs-allow.
It seems like direct access via @tmeasday, why are these set up as absolute paths rather than perhaps module names which could be resolved as normal imports? I'm not really sure if there's a way we can convert from |
|
I believe we need to use the full path to But I guess you are really asking about the absolute path. @ndelangen or @shilman can probably speak to how necessary that is. |
|
To hazard a guess I would say it's due to tools like pnpm which don't let you require a package you don't depend on unless you provide a absolute path (do I have that right?) |
Closes #305 This reverts #289, and fixes it a better way, by replacing filesystem imports (`/@fs/` style absolute path imports) with normal node_modules style imports. So, instead of: ``` /@fs/Users/ianvs/code/experiments/sb-builder-vite-no-container/node_modules/@storybook/vue3/dist/esm/client/preview/config ``` We'll import from ``` @storybook/vue3/dist/esm/client/preview/config ``` Which vite can correctly pre-bundle and add a browserHash to. To test this, run the vue example in this repo. You should see some messages about new dependencies being optimized in the terminal, and the storybook should load up just fine with no errors about `No docs.container set`. I tested this out back to our minimum supported versions of vite (2.5.2) and storybook (6.4.0). It worked fine, although at that old version, one page refresh was required on the very first startup (due to changes in pre-bundled deps). This doesn't happen in vite 2.9.0, which has better support for finding new deps.
Fixes #327 This reverts #323. While I still think it would be great to avoid including deps in our `optimizeDeps.include` as much as we can, it turns out that it's still necessary for a few reasons. 1) If an addon (like `storybook-dark-mode`) publishes a cjs package that `require()`s from a storybook package that has been excluded from optimization, then esbuild seems to not replace that require at build time as it normally would. >Some background: esbuild's bundler emulates a CommonJS environment. The bundling process replaces the literal syntax require(<string>) with the referenced module at compile-time. (https://github.com/evanw/esbuild/blob/master/CHANGELOG.md#01229) This results in the situation outlined in #327. 2) Even after I removed all the packages from `exclude`, if I didn't add them back to the `include` list, there were multiple page refreshes while the app booted the first time, while vite found new deps and reloaded the page. That would only happen once until the cache was populated, which isn't horrible, but it's not a great experience either. 3) Lastly, if I tried to keep the logic which prevented optimizing the storybook framework package, I ended up with react hook warnings, which usually means that multiple versions of react are loaded in the page. That was the final straw which made me decide to completely revert the PR and go back to what we had before. When I did that, my storybook worked just fine. I think we can take another stab at avoiding optimization another time, but we've already made a lot of changes to this package lately and it doesn't seem to be causing problems like I initially suspected in #305.
This reverts commit #289, and fixes storybookjs/storybook#17773 a different way.
@tmeasday and I determined that the
start()function in app/vue3/src/client/preview/index.ts was being called twice, once with all the parameters, and once with just the framework renderer. Eventually I realized that one of the urls calling start was something likehttp://localhost:6006/node_modules/@storybook/vue3/dist/esm/client/preview/index.js?v=afbc4050and the other washttp://localhost:6006/node_modules/@storybook/vue3/dist/esm/client/preview/index.js(notice nov=browserHashat the end). It seems this is due to several things:/@fs/syntax to point to a file on the filesystem, rather than a node_module (maybe vite doesn't automatically append the browser hash in this case? Maybe @patak-dev can confirm.)@storybookpackages, even though there were esm exports available to import. I'm not sure why these are being pre-bundled, but excluding them was necessary to avoid the double-start.The "fix" here is a bit of a hack, and I'd like to find a better way. I'm grabbing the
browserHashfrom a private property of the server (not sure what versions of vite this even works with), and manually appending it to the filenames that we're generating. That alone wasn't enough to fix the problem, though, as I also needed to prevent the@storybookpackages from being optimized.Lastly, this only works if one more change is made to storybook frameworks, to change this line (and it's equivalent in each framework) from
to
Because
@storybook/core/client.jsis a cjs file.I hope that maybe some of the vite folks can help suggest a better way than manually appending a browserHash. I may need to put together a simpler reproduction of the issue that better demonstrates what's going on, but I'm not convinced I'll be able to.
Or, maybe we can change / resolve the absolute paths into normal bare imports and allow the typical import resolution to occur? I.e. change from:
to something like:
But I think that would require a change to core storybook, and might break the webpack builders.