Skip to content

Conversation

@florian-lefebvre
Copy link
Member

@florian-lefebvre florian-lefebvre commented Apr 18, 2025

The PR is very big! I suggest you checkout the branch locally and go through the code because the diff is hard to analyze at the first glance.

Changes

  • Refactors how the experimental fonts API work
  • As I developed the feature, I kept adding stuff on top of stuff etc and it got quite messy, because I discovered things along the way
  • With all what has been done in mind, I've redone a huge part of it so responsabilities are split correctly
  • Now, most things accept "dependencies" (ie. each set a contract) so they can be more easily tested
  • This refactor is also done to prepare for when some of the logic will be extracted to @astrojs/vite-plugin-fonts
  • Along the way, I fixed some bugs and made improvements:
    • There were issues with quoting when family names contained spaces, this is fixed
    • The local providere was preloading too many font files, this is fixed
    • The CSS generated during the build is not minifed for performance

Testing

As a result, there are more/better tests which is always good!

Docs

No behavior change so no docs change! Changesets tho

@florian-lefebvre florian-lefebvre marked this pull request as ready for review April 25, 2025 13:45
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Honestly, it's very hard to find the bug fixes in this huge refactor. As long as tests pass, it's fine. However, I don't have the energy to look at all the code. Take this review with a grain of salt and don't count on it, please.

Copy link
Member

Choose a reason for hiding this comment

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

The folders implementations/ and logic/ have an unusual name. We usually name the folders by feature, but in this case, it's hard to understand which "feature" belongs to. Maybe consider a main folder called fonts/ and then have everything there

@florian-lefebvre

This comment was marked as outdated.

@florian-lefebvre florian-lefebvre merged commit a7b2dc6 into main Apr 28, 2025
16 checks passed
@florian-lefebvre florian-lefebvre deleted the feat/fonts-refactor branch April 28, 2025 10:08
@astrobot-houston astrobot-houston mentioned this pull request Apr 28, 2025
openscript pushed a commit to openscript/astro that referenced this pull request Sep 12, 2025
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.

2 participants