-
-
Notifications
You must be signed in to change notification settings - Fork 6.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
feat(css): support resolving stylesheets from exports map #7817
Conversation
Thanks for creating a PR for this. I've placed this in the to-discuss list for the coming team meetings, as supporting webpack's special conditions is a new feature. Will give an update when we come around to it! |
729a6e1
to
23df103
Compare
How is it looking for getting this feature into Vite 3? |
This is currently still in the team discussion board. We haven't got a chance to visit yet, but will make an update when we do! |
Has this feature been discussed yet? Would also love to see this generally available |
@bluwy any updates here? |
@bluwy What discussion does this feature still need? I would really like to move some projects over to Vite but this feature is important given how all of our existing code imports stylesheets w/ webpack. |
One of the things I'm concerned of that I want to discuss with the team is whether Vite should follow this convention. It seems to only be implemented by Webpack and so a package that takes advantage of this condition currently works with Webpack only. This can be workaround by splitting the modules as separate entrypoints too, and I think that's the better/safer way instead of mixing styles and JS. Others may have different opinion which I'd be interested to hear. I'm trying to push into reviewing more topics in the meetings but we constantly have higher priority topics that pushes back many outstanding backlog. I'm not a fan of this PR being stalled for too long too but we're trying our best to fit in time to discuss things. |
I think keeping styles and js in the same module has huge value for component libraries |
@bluwy, You should consider this feature because most of the libraries build with webpack using this configuration and If a developer tries to use these libs and build it using vite then build will always fail. Is there any workaround that can be used in vite configuration to fix this issue? Because we can not remove sass module exports from third party libraries. @kherock, are you using any workaround to fix this issue without changing anything in library exports? |
Yes, given #11947 I'm thinking this make sense now. I'll bring this to discussion async this week. |
After last week's meeting, we decided to go forward with the new conditions, because we also have the same ones for I'll make some changes in this PR and it should be good for Vite 4.2 |
I believe isUnsafeExport can be dropped as long as However, given the current support for export maps with styles (i.e. none), I don't see this as potentially breaking. I would still encourage its use here since it guards stylesheet imports from doing something invalid like attempting to load a JS file as a stylesheet. |
But preserving resolving these incompatible conditions do give better error messages though, so you'd get something like So I'm kinda thorn on this one, I'll ping the others to see what they think. |
That makes sense, thanks for considering it. I'll let you sort out the DX! I agree that resolution failures tend to be really opaque. But it would be great if something like this were possible!
|
We've discussed this and decided to keep |
Description
This allows stylesheets to import styles that should resolve to a
style
condition in an export map. Resolves #7809.Additional context
I would have preferred to implement the value of the of
unsafe
condition to be true only when either the importer isn't an ES module or when there is an import assertion/query param indicating that the requested module isn't JavaScript (for the case where the importer isindex.html
). However this approach felt a bit too involved, so this newisUnsafeExport
resolver option feels like the best solution for now.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).