chore(fonts):⚡reduce font payload #1999
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🚦 Lighthouse Results (Mobile & Desktop)
|
ShubhamOulkar
changed the title
Need to remove spaces to edit comment correctly. Also need to increase wait time for Netlify preview. Happy 😃 mobile results are improving. |
ShubhamOulkar
changed the title
|
I just realized that for improving performance on mobile needs refactoring in logic and assets file. It should be good to do it in other PR. |
.github/workflows/lighthouse.yml
Outdated
| - name: Skip if only .md files changed | ||
| id: file_check | ||
| run: | | ||
| CHANGED_FILES=$(git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.event.pull_request.head.sha }}) | ||
| echo "Changed files:" | ||
| echo "$CHANGED_FILES" | ||
|
|
||
| NON_MD_FILES=$(echo "$CHANGED_FILES" | grep -vE '\.md$' || true) | ||
|
|
||
| if [ -z "$NON_MD_FILES" ]; then | ||
| echo "Only .md files changed. Skipping job." | ||
| echo "skip_lighthouse=true" >> "$GITHUB_ENV" | ||
| else | ||
| echo "skip_lighthouse=false" >> "$GITHUB_ENV" | ||
| fi | ||
|
|
||
| - name: Exit early if skipped | ||
| if: env.skip_lighthouse == 'true' | ||
| run: echo "Skipping Lighthouse audit because only .md files changed." && exit 0 |
There was a problem hiding this comment.
I’m not comfortable with this. pull_request_target has full permissions, and I don’t want to introduce vulnerabilities into this repository. I’m even thinking it might be better to handle the Lighthouse integration from Netlify.
Also, I feel that Lighthouse doesn’t provide very reliable data, in every PR I’ve seen it run, the results can be quite inaccurate, even when only a markdown file was changed.
There was a problem hiding this comment.
Also, I feel that Lighthouse doesn’t provide very reliable data, in every PR I’ve seen it run, the results can be quite inaccurate, even when only a markdown file was changed.
Our assets and website design logic causing this issue. I think we should get same result in netlify integration because netlify use lighthouse to generate results. ref
handle the Lighthouse integration from Netlify.
I support this and suggested before in my previous PR as well, we should consider netlify option just because we don't want unnecessary CI maintenance.
I don’t want to introduce vulnerabilities
That why I prefer pull_request action for simple task like lighthouse. But this action can not comment on PR's. By the way, there should not be a problem with security because I am using sha. I have seen #2031 this PR.
There was a problem hiding this comment.
I'm removing this workflow from this PR. Let's just keep this PR for fonts.
There was a problem hiding this comment.
My latest commit got 100% results for desktop and on the edge for mobile. 😃
@bjohansebas, I got this woff2 fonts from https://github.com/fontsource/font-files/tree/main/fonts/variable/open-sans/files |
|
Fontsource provides npm packages for the fonts: https://fontsource.org/fonts/open-sans/install |
Hi @Phillip9587, we don't use any bundler in this project so that we can not use this. See Prerequisites section. Fontsource provide CDN option, but I prefer self-hosting fonts. Tell me if I am missing something because you're suggesting this second time I guess. |
page load time reduced by ~300-400ms
~230-270 kB reduction in font payload
Font source: https://github.com/fontsource/font-files/tree/main/fonts/variable/open-sans/files