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(cli): rework font downloading #733

Merged
merged 5 commits into from
May 27, 2023
Merged

Conversation

Sh4rK
Copy link
Contributor

@Sh4rK Sh4rK commented May 27, 2023

Fixes the bug that caused the non-variable Google fonts to be downloaded twice, to the same target file, causing indeterministic file changes and slightly broken font files. See #712 for more context.

Fixes #712.

I ran my WOFF size checking script on the output of this PR, and all the previous errors are gone (except manual fonts), so it seems to work.

There are some unexpected changes relative to font-files@6332265:

Some fonts are completely missing, but they also don't show up on the Google Fonts website for me:

  • fonts/google/arima-madurai
  • fonts/google/fredoka-one
  • fonts/google/gentium-book-basic
  • fonts/google/kantumruy
  • fonts/google/merienda-one

A subset is missing from one font, but again, it doesn't show up in the raw metadata for me either:

  • fonts/variable/radio-canada/files/radio-canada-canadian-aboriginal-standard-italic.woff2
  • fonts/variable/radio-canada/files/radio-canada-canadian-aboriginal-standard-normal.woff2
  • fonts/variable/radio-canada/files/radio-canada-canadian-aboriginal-wdth-italic.woff2
  • fonts/variable/radio-canada/files/radio-canada-canadian-aboriginal-wdth-normal.woff2
  • fonts/variable/radio-canada/files/radio-canada-canadian-aboriginal-wght-italic.woff2
  • fonts/variable/radio-canada/files/radio-canada-canadian-aboriginal-wght-normal.woff2

@changeset-bot
Copy link

changeset-bot bot commented May 27, 2023

🦋 Changeset detected

Latest commit: 7d203d1

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

This PR includes changesets to release 1 package
Name Type
@fontsource-utils/cli 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

@Sh4rK
Copy link
Contributor Author

Sh4rK commented May 27, 2023

Okay, I completely forgot about the tests, give me some more time :D

@ayuhito
Copy link
Member

ayuhito commented May 27, 2023

Thanks for making this PR! 🎉 I'll wait to review after the tests are written so I have a better idea of what the expected outputs are. 👍

There are some unexpected changes relative to font-files@6332265:

Google changes things a lot which includes removing things so this should be a non-issue. See related: #578

Fixes the bug that caused the non-variable Google
fonts to be downloaded twice, to the same target file,
causing indeterministic file changes and slightly
broken font files. See fontsource#712 for more context.

Fixes fontsource#712.
@Sh4rK
Copy link
Contributor Author

Sh4rK commented May 27, 2023

Should I also add a changeset? If yes, is this a patch level change?

Copy link
Member

@ayuhito ayuhito left a comment

Choose a reason for hiding this comment

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

Huge improvement from before! Thank you! A small suggestion, but otherwise the code looks great. 🎉

I also went ahead and made the changeset, so you don't need to worry about that.

packages/cli/src/google/download.ts Outdated Show resolved Hide resolved
Copy link
Member

@ayuhito ayuhito left a comment

Choose a reason for hiding this comment

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

Everything looks good! Once again, thank you for making this PR! 🎉

@ayuhito ayuhito merged commit 0f162c9 into fontsource:main May 27, 2023
1 check passed
@Sh4rK Sh4rK deleted the fix/font-download branch May 27, 2023 19:25
@github-actions github-actions bot mentioned this pull request May 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate Changing WOFF Files When Rebuilding Repository
2 participants