-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Core: Restore /preview
etc package exports; return unresolved path from presets.
#19045
Core: Restore /preview
etc package exports; return unresolved path from presets.
#19045
Conversation
Allow `preview.js` etc to be the name of the package exports
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.
LGTM. @ndelangen we should probably figure out how to clean this up somehow -- maybe in Vite, which was the reason for the change in the first place?
The reason vite is importing the files with |
Instead, just return the import path unresolved
All of Node, Webpack and Vite can `import('<pkg>/preview')` etc, as long as the `exports` map is there and setup like so. If the `exports` map is not there, then they can import `node_modules/<pkg>/preview.js` instead. The issue occurred when an export map existed but Vite tried to import a path that wasn't defined in there.
/preview
etc package exports; return unresolved path from presets.
@ndelangen -- @shilman and I reworked this, and we are happy with this solution now, it resolves (haha) everything, apart from one possible wrinkle (see above):
|
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.
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.
This is great, and means I think we can get rid of our custom utility that converted absolute paths back into "bare" imports.
As discussed, hold your horses on this, absolute paths are coming back!
That's correct. But I am going to tweak it to absolutize the path (but not resolve the export). The rationale is that because addons can reference each other by name (e.g. in |
This is weird... so sb-bench IS running on localhost, but not reachable (assumed)? |
Looks like when I fixed it and merged it to next next was already broken. |
argh, seems it's not consistent @tmeasday nothing changed between the CI being green and showing the error again. What a pain. |
Seems like the error is not related to this PR though, right? Any reason not to merge it? Edit: I guess the chromatic snapshots are still failing, which as I understand was the reason for this PR. |
Issue:
Chromatic regressions introduced in #19007, ultimately due to many addons not working in example apps.
The root issue that was being worked around was that if a package has an
exports
field, Vite can only import an entry there. i.e. it canimport('@storybook/addon-backgrounds/preview')
notimport('@storybook/addon-backgrounds/dist/preview.js')
, which is what the presets code was resolving it to.What I did
In the presets code, try resolving an import path, but return the import path unmodified, so the builder can import it itself.
NOTE: This means that import paths (e.g.
previewEntries
) will be of the form'@storybook/addon-backgrounds/preview'
rather than/Users/xyz/..../node_modules/@storybook/addon-backgrounds/preview
. Is this a problem @ndelangen? If so we could "half resolve" the import (ie. absolutize the import path).How to test