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

[gatsby-plugin-preload-fonts] Add option to skip checking if fonts change - to use in CI #20185

Closed
masives opened this issue Dec 18, 2019 · 9 comments
Labels
stale? Issue that may be closed soon due to the original author not responding any more.

Comments

@masives
Copy link
Contributor

masives commented Dec 18, 2019

Summary

gatsby-plugin-preload-fonts requires user action if fonts are detected to be unchanged. This makes it impossible to use with any CI tool as it will hang the build. It would be useful to have an option to provide a flag/env variable that would skip this check or force it to be done again.

Basic example

Currently for each page hash is created a

const {GATSBY_PLUGIN_PRELOAD_FONTS_FORCE} = process.env

const shouldForceCrawl = GATSBY_PLUGIN_PRELOAD_FONTS_FORCE

const hash = crypto.createHash(`md5`)
  routes.forEach(r => hash.update(r))
  if (cache.hash === hash.digest(`hex`)) {
    const lastRun = formatRelative(new Date(cache.timestamp), new Date())
    const ok = shouldForceCrawl ? true : await logger.confirm(`

  ${blue(`note`)} routes have not changed from the last run; if you haven't
       added any new routes or font requirements since then, you
       should be good to go! would you like to crawl them anyways?

         - ${dim(`last run`)} ${bold(lastRun)}
         - ${dim(`route hash`)} ${bold(cache.hash)}

`)
    if (!ok) process.exit(0)
  }

If the proposal involves a new or changed API, include a basic code example. Omit this section if it's not applicable.

Motivation

To enable using this plugin in CI

I can provide PR

@pieh
Copy link
Contributor

pieh commented Dec 18, 2019

We do have isCi utility that could be used to automatically make some default decision as well. (so it just works with sane default and doesn't hang)

I honestly am not quite sure the why we even need to prompt user in the first place. @superhawk610 can you provide some context around this? This get's prompted when hash matches so instinctively I would say that not re-reunning should be default (unless there are subtle things that don't quite work, which I'm not aware of). I tried to look over discussion in #14608 but I don't think this was ever target of discussion there

@masives
Copy link
Contributor Author

masives commented Dec 23, 2019

@pieh I'm afraid this will not work as gatsby-preload-fonts script that uses puppeteer is ran outside of gatsby build. This happens before the build and then gatsby-plugin-preload-fonts uses it's result in the build process

@github-actions
Copy link

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 12, 2020
@masives
Copy link
Contributor Author

masives commented Jan 14, 2020

Could you please suggest if I should make this pull request or should we abandon this as unneeded feature?

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 14, 2020
@superhawk610
Copy link
Contributor

Hey everyone, sorry for the delay in chiming in. Let me explain a bit about my reasoning for keeping this outside of CI, hopefully that will provide some clarity.

There are 3 situations where you need to run gatsby-preload-fonts to regenerate the font cache:

  • after you have manually added new routes (usually by adding a new file in the pages directory)
  • after you’ve added a new plugin that generates new routes (e.g. source plugins, etc)
  • after you’ve added new fonts that require preloading to existing routes

Gatsby can automatically detect new routes (the first situation), which is what we use to check/update the cache hash. However, the 2nd and 3rd situation are too high-level to be automatically detected and thus must be manually enforced by the developer.

Detecting fonts that need to be preloaded can become quite time consuming for any project larger than a few routes, often taking 10-15 minutes or more (it has to crawl every route in your site, dynamic and static). As such, I believe rerunning this script on every CI build will very often simply waste resources without any actual benefit.

That being said, I do think rerunning this script anytime the cache hash changes (whenever new routes can be detected by Gatsby) would be useful and wouldn’t needlessly waste resources. @masives if you’d like to open a PR to add a non-interactive mode for gatsby-preload-fonts in CI environments whenever the cache hash has changed, I believe that would be a useful addition.

@pieh does that help clear things up?

@masives
Copy link
Contributor Author

masives commented Jan 22, 2020

@superhawk610 Thanks for the explanation, I'll draft a PR for this in this or next week.

@masives
Copy link
Contributor Author

masives commented Jan 30, 2020

@pieh I've drafted PR #21037

@github-actions
Copy link

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Feb 19, 2020
@github-actions
Copy link

github-actions bot commented Mar 1, 2020

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜

@github-actions github-actions bot closed this as completed Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale? Issue that may be closed soon due to the original author not responding any more.
Projects
None yet
Development

No branches or pull requests

4 participants