-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[7/n] @next/font/google: Add font weight, style to css and js properties #2963
[7/n] @next/font/google: Add font weight, style to css and js properties #2963
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Ignored Deployments
|
FontWeights::Fixed(weights) => Ok(FontAxes { | ||
wght: weights.clone(), | ||
wght: IndexSet::from_iter(weights.iter().map(|w| w.to_string())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the weights already sorted? Should this just be a vec instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these sets are only ever iterated (membership is never checked). I went with the IndexSet here because of the invariant that there can't be duplicated values.
At this point FontWeights
also being a set would take care of duplicates from the query string, which itself is derived from how the user invoked the font function. So in practice it's probably not an issue outside of programming errors. Does AutoSet
preserve insertion order?
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
Benchmark for 323f17e
Click to view full benchmark
|
🟢 CI successful 🟢Thanks |
6cd213a
to
d08a0a8
Compare
Benchmark for 368bc65Click to view benchmark
|
d08a0a8
to
b7ee981
Compare
Benchmark for 5fae10fClick to view benchmark
|
@@ -76,11 +79,27 @@ impl ImportMappingReplacement for NextFontGoogleReplacer { | |||
r#" | |||
import cssModule from "@vercel/turbopack-next/internal/font/google/cssmodule.module.css?{}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we putting this into @next/font/google/cssmodule.module.css
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all virtual assets and will never exist on disk at those locations. I figured it might be confusing or misleading to present them as if they did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the whole @next/font
package virtual?
looking at https://github.com/vercel/next.js/blob/canary/packages/font/google/index.js
b7ee981
to
808dd46
Compare
Benchmark for 34c135fClick to view benchmark
|
This adds
font-weight
,font-style
, and a fallback font both to the css styled by the exportedclassName
as well as to the exportedstyle
object.Test Plan:
Given
const inter = Inter({weight: "500", fallback: ["Arial"]});
, verified the following css properties were generated and the following js was exported: