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

Fix VITE_ASSET bug #3439

Merged
merged 23 commits into from
May 26, 2022
Merged

Fix VITE_ASSET bug #3439

merged 23 commits into from
May 26, 2022

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented May 24, 2022

Changes

  • Closes 🐛 BUG: css url() replaced by __VITE_ASSET__* #2146
  • Removes most of the logic in our CSS plugin. Instead we defer to vite:css-post now which does CSS bundling and updating of JS.
  • We now use manualChunks to create chunks for CSS. The algorithm walks a CSS file until it finds its parents and creates a hash from that. This means that if CSS is shared on multiple pages it will be combined into a shared chunk.
  • The one nasty pain point is that an upstream Vite bug forces us to regex replace import statements so that the css-post plugin will correctly remove them. I'll submit an upstream PR to fix that.

Testing

  • Many tests that have changed are because they depended on the output being ran through esbuild and now that we are deferring to vite:css-post this happens in a different way and the output is not different.
  • One test suite is no longer relevant (astro-css-bundling-import) because it depends on ordering that we no longer control, so it was removed.
  • Some tests expected multiple CSS files but we have no combined into one, so the tests are changed to check the contents instead.
  • Tests added to verify that importing fonts from a package in CSS now works (this was the thing VITE_ASSET prevented.

Docs

N/A, bug fix.

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2022

🦋 Changeset detected

Latest commit: 3efcc62

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

This PR includes changesets to release 1 package
Name Type
astro Patch

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 May 24, 2022
@matthewp matthewp marked this pull request as ready for review May 25, 2022 15:52
Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

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

lgtm! Great to see more of the CSS work handed off to Vite, and the shared bundles for page-level CSS is great to see 👍

packages/astro/src/vite-plugin-build-css/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks good—just want to check out one thing locally, but I'll be quick about it.

This is huge! Great work. ❤️

packages/astro/src/vite-plugin-build-css/index.ts Outdated Show resolved Hide resolved
packages/astro/test/0-css.test.js Outdated Show resolved Hide resolved
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great!

@matthewp matthewp merged commit ac3c60d into main May 26, 2022
@matthewp matthewp deleted the vite-asset branch May 26, 2022 18:00
@github-actions github-actions bot mentioned this pull request May 26, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* Fix VITE_ASSET bug

* Updated test that depended on esbuild output

* Fix some more tests

* Fix css config and postcss tests

* Git client only working

* Fix static build test

* Update tailwind tests

* Fix build

* Fix css bundling tests

* Updated some more tests for windows

* Remove tests that are no longer relevant

* Cause it to break

* Fix bug and add explanation

* Adds a changeset

* Inline comments about what the hashing is doing

* Update packages/astro/src/vite-plugin-build-css/index.ts

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>

* Update to the lockfile

* Minify css

* Update tailwind tests

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
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.

🐛 BUG: css url() replaced by __VITE_ASSET__*
3 participants