feat(fonts): vite plugin #13093
feat(fonts): vite plugin #13093
Conversation
There was a problem hiding this comment.
Added some more nits, and I probably spot a bug. Nothing is a blocker, great work @florian-lefebvre !
|
|
||
| export const GOOGLE_PROVIDER_NAME = 'google'; | ||
|
|
||
| // TODO: https://github.com/unjs/unifont/issues/108 |
There was a problem hiding this comment.
Consider, that the issue has been stuck for two weeks, maybe we should consider adding a warning or something for users.
| */ | ||
| type PreloadData = Array<{ | ||
| /** | ||
| * Absolute link to a font file, eg. /_astro/fonts/abc.woff |
There was a problem hiding this comment.
The example doesn't point to an absolute file path. Usually it's something that starts with /Users or C:\
There was a problem hiding this comment.
I mean an absolute link, not an absolute filepath. That was to emphasize the fact it starts with /, not ./ or anything else. if you have a better name, I'll take it!
| /** | ||
| * A font type, eg. woff2, woff, ttf... | ||
| */ | ||
| type: string; |
There was a problem hiding this comment.
nit: extension? Or, if it's really a type, maybe we could consider an enum
There was a problem hiding this comment.
I prefer to call it type as it's used in the type prop of the link element in the Fonts component. And I think it's not a 1-1 match, I'm using a naive approach atm i'll add a todo to investigate
| subsets: family.subsets ?? DEFAULTS.subsets, | ||
| fallbacks: family.fallbacks ?? DEFAULTS.fallbacks, | ||
| }; | ||
| const { fonts: fontsData, fallbacks } = await resolveFont(family.name, resolvedOptions, [ |
There was a problem hiding this comment.
I'll use fallbacks later
| // 1. local and has a name | ||
| // 2. remote and has an url | ||
| // Once we have the key, it's safe to access the related source property | ||
| const key = 'name' in source ? 'name' : 'url'; |
There was a problem hiding this comment.
Small nit: explain why name has precedence over url
There was a problem hiding this comment.
It doesn't matter which would should be accessed first, it's purely arbitrary
| if (!hashToUrlMap.has(hash)) { | ||
| hashToUrlMap.set(hash, value); | ||
| const segments = hash.split('.'); | ||
| // It's safe, there's at least 1 member in the array |
There was a problem hiding this comment.
| // It's safe, there's at least 1 member in the array | |
| // It's safe, there's at least 1 member in the array because the hash has a pattern <HASH>.<EXTENSION> |
| const type = segments.at(-1)!; | ||
| if (segments.length === 1 || !FONT_TYPES.includes(type)) { |
There was a problem hiding this comment.
The comment on line 196 is in contradiction with segments.length === 1. The check will always be false because segments will always have two items: the hash and the extension/type. This is a bug?
There was a problem hiding this comment.
I tested manually this part and basically consider those cases (assuming a split on dots):
""=>[""](length 1 invalid)"abc"=>["abc"](length 1 invalid)"abc.def"=>["abc", "def"](length 2 valid, last member not matching font type)"abc.woff"=>["abc", "woff"](length 2 valid, last member matching font type)"abc.def.ghi"=>["abc", "def", "ghi"](length 3 valid, last member not matching font type)"abc.def.ttf"=>["abc", "def", "ttf"](length 3 valid, last member matching font type)
Changes
FontscomponentTesting
Manual + automated
Docs
N/A