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

Allow vite to refer to inlined CSS #8351

Merged
merged 6 commits into from
Sep 5, 2023

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Sep 1, 2023

Changes

Closes #8329
Inlined CSS chunks are not deleted anymore, because when using dynamic imports with import.meta.glob(), vite tries to preload them.

Testing

Added a fixture.

Docs

Does not affect usage.

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2023

🦋 Changeset detected

Latest commit: 443f15d

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 pkg: svelte Related to Svelte (scope) pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Sep 1, 2023
@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 1, 2023

I'm hoping there's a better way.

Ideally vite would not attain references to files we might later delete. Alternatively, it could not throw an error when preloading fails as it is not critical.

I will try to look around their codebase to find something useful. If I don't, I'll mark this ready to review on monday.

@lilnasy lilnasy force-pushed the inline-css-dangling-references branch 2 times, most recently from 1e5cd88 to 5929ad3 Compare September 4, 2023 14:13
@lilnasy lilnasy marked this pull request as ready for review September 4, 2023 14:13
@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 4, 2023

I haven't had time to look into fixing it on the vite side, but the compromise is small - small css files included in the server bundle.

And the unnecessary requests vite makes is a minor annoyance. It does not affect performance.

@lilnasy lilnasy force-pushed the inline-css-dangling-references branch from 5929ad3 to 2daf995 Compare September 4, 2023 14:39
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.

I'm ok with this fix for now. If you'd like to look further, I think there's maybe a couple ways for a direct fix:

  1. Use modulePreload.resolveDependencies to filter out the removed CSS. A bit tricky to bridge this config option to generateBundle in plugin-css.
  2. Filter out the deps here that does not exist in the bundle. Could be a better fix.

@lilnasy lilnasy force-pushed the inline-css-dangling-references branch from 2daf995 to 443f15d Compare September 5, 2023 16:47
@lilnasy
Copy link
Contributor Author

lilnasy commented Sep 5, 2023

@bluwy
I looked into 1, but css is special-cased and preloaded anyway - build.modulePreload only applies to scripts.

I will try to look into 2 later this week, but I am skeptic because vite's built-in hooks run before app hooks IIRC, so I expect it to have already created the preload script by the time.

Either way, not blocking. Will merge after node 18 tests pass.

@lilnasy lilnasy merged commit 7d95bd9 into withastro:main Sep 5, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 5, 2023
@bluwy
Copy link
Member

bluwy commented Sep 6, 2023

@lilnasy thanks for looking into it.

I will try to look into 2 later this week, but I am skeptic because vite's built-in hooks run before app hooks IIRC, so I expect it to have already created the preload script by the time.

Vite handles that through plugins, so as long as we order ours right before Vite's, I think it could work. You can check out the plugin ordering at here and here. Not urgent of course, feel free to look if you're interested!

@lilnasy lilnasy deleted the inline-css-dangling-references branch September 20, 2023 14:20
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) pkg: integration Related to any renderer integration (scope) pkg: svelte Related to Svelte (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Unable to preload CSS when using import.meta.glob
3 participants