-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Vite: Revert: "Improve handling of preview annotations" #30789
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
Conversation
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.
7 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| // eslint-disable-next-line no-underscore-dangle | ||
| const preview = (window as any).__STORYBOOK_PREVIEW__ as PreviewWeb<ReactRenderer> | undefined; | ||
| const channel = (window as any).__STORYBOOK_ADDONS_CHANNEL__ as Channel | undefined; |
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.
logic: Moving these variables outside the loader function could cause issues if they're needed before the window object is fully initialized
|
View your CI Pipeline Execution ↗ for commit d57b0a4.
☁️ Nx Cloud last updated this comment at |
Does it work if you explicitly transpile it to esm? How does one reproduce this error? |
|
@tobiasdiez You can create svelte sandbox in the monorepo with this command: Or open it in stackblitz: |
|
@tobiasdiez This seems to fix it: It seems that we are now forced to add all CJS dependencies to optimizeDeps. While before #28798 we only needed that as a perf optimization. |
|
Since this code is generated by a plugin, it makes sense that vite cannot find these imported packages during the initial screening of the dependencies. Thus, in either case, it is a good idea to add these dependencies to Is this behavior particular to the |
It seems with all, I also had to add:
So because we only test one sandbox (react-vite) in dev mode, I'm afraid this will easily regress, if someone adds a dependency. |
Vite: Add new dependencies to Vite optimizeDeps to fix #30789
Reverts #28798
That PR introduced a critical problem, that causes all Svelte stories to never load in dev mode (works fine in a built Storybook).
The Preview would have an infinite loading spinner, and the console would have an error:
I'm still not sure why this happens, nor how to fix it, so for now we just have to revert the PR.
cc @tobiasdiez
Additional Context
axobject-queryis a dependency of Svelte:This problem has shown up in other places too, but with no clear solution:
Diving into the Network request in the browser, this is the content of
'/node_modules/axobject-query/lib/index.js?v=d4376a8f':My hunch says this might be a CJS/ESM issue, but I could be wrong.
Greptile Summary
This PR reverts changes to preview annotation handling in the Vite builder due to critical issues causing Svelte stories to fail loading in development mode.
code/builders/builder-vite/src/codegen-modern-iframe-script.tsto use dynamic imports instead of static importsaxobject-querydependency that caused infinite loading spinnercode/builders/builder-vite/src/optimizeDeps.tsto remove 'react-dom/test-utils' and 'semver' from pre-bundling