-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-preload-fonts): preload used fonts #14608
Conversation
I'm currently working on unit tests but I figured I could go ahead and get some feedback on the plugin layout/implementation - in particular, I'm curious how my approach to the following tasks fit the Gatsby ecosystem:
|
Subscribed to this! Thanks for taking this on @superhawk610 |
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.
Thanks for taking this one on! Looks pretty good.
I wonder if we can implement this as a real gatsby plugin instead of a seperate script. This gives us the opportunity to use the gatsby-cli reporter & gatsby cache.
We could look into https://www.gatsbyjs.org/docs/node-apis/#onCreateDevServer to run puppeteer when the dev server is running.
What do you think?
@wardpeet I went with a separate script to follow the idea proposed in the original linked issue; I'm cool with either integrating with the dev server directly or having that as an option, my only concern is that this is a costly operation (minutes on smaller projects and potentially hours on larger projects). I added some basic route caching to prevent crawling if the list of routes hasn't changed, perhaps I should make that more granular and only crawl new routes by default? cc @KyleAMathews: what are your thoughts on having the font-scraping script run onCreateDevServer vs manually via a package script? |
Yeah — my guess that this would be a really costly operation so best done in a periodic cron job — it looks like your testing has confirmed that so let's keep it a separate script. |
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.
Looking good!
Could you include a screenshot of what a run of the script looks like?
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.
This looks really good! I added a few comments/questions but this is ready to go! Well done! 👏
Hey, sorry about the delay, I put this on the backburner until #15475 was addressed but it never was. I'll try and get a commit addressing @wardpeet's most recent feedback this week, hopefully the versioning thing will have sorted itself out. EDIT: I'm beginning a new job this week, so I haven't had any free time to work on this. I'll get final review changes up as soon as I get the time :) |
Oh yeah, it got fixed. I missed your issue. It would be awesome to get this one shipped 🤗. |
@wardpeet Should be g2g 🔥 |
Awesome! 🔥 I'll fix tests and give this plugin one more try and i'll merge afterwards! Thanks for your hard work. This is going to be awesome! |
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.
🤞 I believe this one is going to be the one. Tests node 8 works so. Thanks @superhawk610 for this plugin. Like I said before, this is awesome!
@superhawk610 we have published your plugin |
Is this recommended for self-hosted fonts? |
@universse Yeah, should be beneficial whether self hosted or not |
Cause I was trying to run the script
So I started dev server, and run the scripts, which gives me this output // font-preload-cache.json
{
...
"assets": {
"/": {
"http://localhost:8000/static/Spectral-ExtraBold-76dd69dbe4b4ddc4f3923bf08e883d43.woff2": true,
....
},
}
} This works if fonts are loaded from external sources since you get absolute external urls. That's not the case for self-hosted ones with relative urls. So I guess there needs to be a way to configure the origin. Not sure if i'm missing anything though. |
@universse good catch, you could add a PR to actually check the url if it matches the dev server we strip out the domain. |
I made a PR #17235 |
Maybe I'm missing something simple, but just curious as to how one is meant to use this with continuous deployment? If I run gatsby develop and then gatsby build locally then all works as expected, but when I push a commit and Netlify starts building my site then it obviously errors with a "err could not establish a connection with the dev server" message. Please explain, as if to an idiot! :) |
You'll need to tweak your CD config to run the dev server in the background, then run |
@superhawk610 Similar question to @chocobuckle here. Any idea how to get this to work on Gatsby Cloud when I deploy? |
@nandorojo I'm not super familiar with Gatsby Cloud configuration, but from the docs it seems like the best way to accomplish this would be to add a
(I haven't tested this but that's the basic idea) |
I'm in the same boat as @chocobuckle . We're hosting on Netlify and the build script is run when we push to branch X, but since the font mapping needs |
@botteu I think you'd need to write a node script that would
|
@masives Yep. But I was hoping to avoid that 😉 But thanks anyways 👍 |
Description
This plugin enables a two-step process for embedding
<link preload>
tags into each statically rendered page in your build output:gatsby-config.js
and these tags will be automagically embedded into each page at build time.TODO
Related Issues
Closes #14281.