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.

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

Merged
merged 3 commits into from Mar 7, 2023

Conversation

kherock
Copy link
Contributor

@kherock kherock commented Apr 19, 2022

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 is index.html). However this approach felt a bit too involved, so this new isUnsafeExport resolver option feels like the best solution for now.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added the p2-to-be-discussed Enhancement under consideration (priority) label Apr 20, 2022
@bluwy
Copy link
Member

bluwy commented Apr 20, 2022

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!

@kherock
Copy link
Contributor Author

kherock commented Jun 15, 2022

How is it looking for getting this feature into Vite 3?

@bluwy
Copy link
Member

bluwy commented Jun 15, 2022

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!

@kherock
Copy link
Contributor Author

kherock commented Sep 25, 2022

Has this feature been discussed yet? Would also love to see this generally available

@kherock
Copy link
Contributor Author

kherock commented Oct 21, 2022

@bluwy any updates here?

@kherock
Copy link
Contributor Author

kherock commented Dec 16, 2022

@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.

@bluwy
Copy link
Member

bluwy commented Dec 17, 2022

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.

@edoardocavazza
Copy link

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 think keeping styles and js in the same module has huge value for component libraries

@sandeeppatidar30
Copy link

sandeeppatidar30 commented Feb 14, 2023

@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?

@bluwy
Copy link
Member

bluwy commented Feb 14, 2023

Yes, given #11947 I'm thinking this make sense now. I'll bring this to discussion async this week.

rcluis
rcluis previously approved these changes Feb 18, 2023
@bluwy
Copy link
Member

bluwy commented Feb 25, 2023

After last week's meeting, we decided to go forward with the new conditions, because we also have the same ones for mainFields. We should not use the isUnsafeExport though, I think it should work without as a general fallback, and it could be a breaking change too if that's used.

I'll make some changes in this PR and it should be good for Vite 4.2

@kherock
Copy link
Contributor Author

kherock commented Feb 25, 2023

I believe isUnsafeExport can be dropped as long as style and others appear before import and require in export maps.

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.

@bluwy bluwy linked an issue Feb 25, 2023 that may be closed by this pull request
7 tasks
@bluwy bluwy added p3-significant High priority enhancement (priority) and removed p2-to-be-discussed Enhancement under consideration (priority) labels Feb 25, 2023
@bluwy
Copy link
Member

bluwy commented Feb 25, 2023

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.

import/require doesn't make sense for styles, but node/browser might, though it sounds kinda silly to use that for styles the more I think about it (or maybe not).

But preserving resolving these incompatible conditions do give better error messages though, so you'd get something like Unable to parse... <some js code>, instead of Unable to resolve subpath './something' in a case where you accidentally import a JS only subpath.

So I'm kinda thorn on this one, I'll ping the others to see what they think.

@kherock
Copy link
Contributor Author

kherock commented Feb 25, 2023

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!

./my-styles.scss: failed to resolve conditions "style", "sass", or "default" in ./node_modules/my-component/package.json:

@use 'my-component';
^^^^^^^^^^^^^^^^^^^^

@bluwy bluwy added this to the 4.2 milestone Feb 26, 2023
@bluwy
Copy link
Member

bluwy commented Mar 7, 2023

We've discussed this and decided to keep import/require/browser/node conditions for CSS for now to be safe. If it turns out to negatively affect things, we could tighten it up in Vite 5 again.

@bluwy bluwy merged commit 108aadf into vitejs:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-significant High priority enhancement (priority)
Projects
Archived in project
6 participants