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

Prevent ?inline and ?raw CSS from being bundled as CSS #6161

Merged
merged 4 commits into from
Feb 7, 2023
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Feb 7, 2023

Changes

Testing

  • Test added for ?inline and ?raw usage.

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented Feb 7, 2023

🦋 Changeset detected

Latest commit: b429a8a

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Feb 7, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was about to suggest the incorrect regex test, but that's fixed now. LGTM once the tests pass.

Also, according to the issue, is this still a Vite bug? I don't think Vite is removing pure css chunks for ?inline 🤔

Also, I noticed the cssRe check can be replaced with import { isCSSRequest } from 'vite' since Vite 4.0. We could clean that up here, or I'm fine tackling that in a separate PR too.

@matthewp
Copy link
Contributor Author

matthewp commented Feb 7, 2023

@bluwy does the vite version of isCSSRequest exclude ?inline and ?raw? If so I agree that it would be good to use that.

This could technically happen in Vite but it would require that an ?inline CSS module be in the same bundle as another (non-inline) CSS bundle and without any JS. It happened to us because we control manualChunks to force all CSS into the common bundle. But it wouldn't happen in normal Vite, again unless someone was using manualChunks.

@matthewp
Copy link
Contributor Author

matthewp commented Feb 7, 2023

Looks like the isCSSRequest in Vite matches any CSS. For our purposes it's really "buildable CSS". I'll update the code to use that.

@bluwy
Copy link
Member

bluwy commented Feb 7, 2023

This could technically happen in Vite but it would require that an ?inline CSS module be in the same bundle as another (non-inline) CSS bundle and without any JS. It happened to us because we control manualChunks to force all CSS into the common bundle. But it wouldn't happen in normal Vite, again unless someone was using manualChunks.

Ah I see. So what happened before is that all types of CSS imports are combined as a CSS chunk, even if they're not really CSS like for ?inline and ?raw. This is kinda an edge case, maybe it's worth taking a look in Vite still, but I think this PR as the fix for now is great too.

@matthewp
Copy link
Contributor Author

matthewp commented Feb 7, 2023

Yep, that's exactly right. I can't think of a scenario where this would happen in regular Vite without someone manipulating manualChunks like we were. In this case the manualChunks part is the error, we don't actually want this type of CSS in that same chunk. If it were it would prevent it from being split into a CSS file!

@matthewp matthewp merged commit f6fc662 into main Feb 7, 2023
@matthewp matthewp deleted the css-inline branch February 7, 2023 15:49
@astrobot-houston astrobot-houston mentioned this pull request Feb 7, 2023
@nicholasray
Copy link

@matthewp It seems like styles imported with the ?raw query param are being bundled in the dev server. For example, in the following codesandbox, I'd expect the background to not be red since those styles are imported with ?raw, but it is red:

https://codesandbox.io/p/sandbox/rough-shape-l7t6qk?file=%2Fpackage.json&selection=%5B%7B%22endColumn%22%3A20%2C%22endLineNumber%22%3A13%2C%22startColumn%22%3A20%2C%22startLineNumber%22%3A13%7D%5D

I noticed this behavior began with Astro 2.0.7. Is that intended?

@bluwy
Copy link
Member

bluwy commented Feb 25, 2023

Seems like a bug to me. Feel free to open an issue so we can track it. It could also likely be a Vite bug but I'm not sure yet.

@nicholasray
Copy link

@bluwy Thank you. I created #6422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build breaks when importing CSS as inline
3 participants