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

Preload FontAwesome, JS and CSS #3057

Merged
merged 25 commits into from
Sep 20, 2021
Merged

Preload FontAwesome, JS and CSS #3057

merged 25 commits into from
Sep 20, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Aug 25, 2021

Fixes #2741

Changes proposed in this pull request:

  • Adds support for preloads in Document
  • Adds preload() and preloads() methods to Frontend extender
  • Adds default preloads for FontAwesome fonts
  • Adds default preloads for core JS and CSS assets

The benefits of preloading FontAwesome are that these would normally only be fetched when CSS parsing is complete, which is often.

Preloading JS and CSS only makes a difference if we're preloading FontAwesome already. Browsers classify preloads as higher priority requests, meaning they would stall downloads of our JS/CSS until these are done. Adding these as preloads prevents this, and they are all fetched at once.

Overall, preloading results in a consistently faster page load time. Almost always 100-150ms faster, sometimes reaching 200ms+. If/when themes and other extensions begin preloading their assets, this could increase further.

Reviewers should focus on:

My failure to write/comprehend PHP and my blatant disregard for helpers and simplicity.

Screenshot

image

With preloads

image

Without preloads

image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

  • Related documentation PR:

src/Extend/Frontend.php Show resolved Hide resolved
src/Extend/Frontend.php Outdated Show resolved Hide resolved
src/Extend/Frontend.php Outdated Show resolved Hide resolved
src/Frontend/FrontendServiceProvider.php Outdated Show resolved Hide resolved
tests/integration/extenders/FrontendPreloadTest.php Outdated Show resolved Hide resolved
src/Extend/Frontend.php Outdated Show resolved Hide resolved
src/Extend/Frontend.php Outdated Show resolved Hide resolved
Co-authored-by: Sami Mazouz <sychocouldy@gmail.com>
@askvortsov1
Copy link
Member

Very excited to see this! Let's fix tests, then good to go IMO

src/Extend/Frontend.php Outdated Show resolved Hide resolved
Co-authored-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9
Copy link
Member

SychO9 commented Sep 1, 2021

I thought that last code suggestion would for sure fix the tests :P

@davwheat
Copy link
Member Author

davwheat commented Sep 1, 2021

Weirdly, it seems to work fine locally.

image
image

@davwheat
Copy link
Member Author

davwheat commented Sep 1, 2021

I don't have time to work on core for the next few days, so if either of you have any ideas, please just push straight to the branch :)

src/Extend/Frontend.php Outdated Show resolved Hide resolved
src/Extend/Frontend.php Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Member

@davwheat @SychO9 issue was with a cast when preloads was called. Fixed now, and changed naming to be more descriptive of structure.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Approving tentative on the singleton comment

Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

Also going to approve tenitive the singleton comment being fixed

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

LGTM

@davwheat davwheat merged commit 88724bb into master Sep 20, 2021
@davwheat davwheat deleted the dw/preload-assets branch September 20, 2021 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement preloading and prefetching
5 participants