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

Remove unused CSS output files when inlined #8743

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Remove unused CSS output files when inlined #8743

merged 1 commit into from
Oct 4, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 4, 2023

Changes

fix #8567

Fixes 2 issues:

  1. Fix Vite preloading CSS references by updating importedCss.
  2. CSS files inlined to any pages should not be deleted.

I'm actually unsure if there's a bug in no2, where in the css-dangling-references fixture, the CSS is not inlined/duplicated to both the pages.

Testing

The existing tests (especially css-dangling-references) should pass

Docs

n/a. bug fix.

@bluwy bluwy requested a review from lilnasy October 4, 2023 10:07
@changeset-bot
Copy link

changeset-bot bot commented Oct 4, 2023

🦋 Changeset detected

Latest commit: b4ef577

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 Oct 4, 2023
// CSS is already added to all used pages, we can delete it from the bundle
// and make sure no chunks reference it via `importedCss` (for Vite preloading)
// to avoid duplicate CSS.
delete bundle[id];
Copy link
Member

Choose a reason for hiding this comment

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

It's better to avoid delete

bundle[id] = undefined

This page contains some information about why: https://biomejs.dev/linter/rules/no-delete

I predict that doing so will trigger the TS compiler, though

Copy link
Member Author

Choose a reason for hiding this comment

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

The convention when dealing with rollup bundles is to delete it rather than setting it to undefined. Otherwise it kinda means that "the chunk exists but it's undefined".

@artursopelnik
Copy link

+1

Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

This is great!
I was unsure about the timing of our "post" plugin against vite's preload plugin, but I can now see that it is "post" as well.

Tested against threlte docs as well, which is where the issue was discovered. Works perfectly!

@lilnasy
Copy link
Contributor

lilnasy commented Oct 4, 2023

  1. CSS files inlined to any pages should not be deleted.

That doesn't sound right.

I'm actually unsure if there's a bug in no2, where in the css-dangling-references fixture, the CSS is not inlined/duplicated to both the pages.

You're right, the fixture does not work how I imagined. The html is not rendered either. I think what we might be noticing is that import.meta.glob() is not compatible with rendering on the server.

@bluwy
Copy link
Member Author

bluwy commented Oct 4, 2023

2. CSS files inlined to any pages should not be deleted.

That doesn't sound right.

Sorry I messed up my wording a bit 😅 I mean CSS files not inlined to any pages should not be deleted. That was why I added the sheetAddedToPage guard.

@bluwy bluwy merged commit aa265d7 into main Oct 4, 2023
13 checks passed
@bluwy bluwy deleted the css-away branch October 4, 2023 13:18
@astrobot-houston astrobot-houston mentioned this pull request Oct 4, 2023
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.

Unnecessary CSS files in dist
4 participants