Switch fontawesome from font files to embedded SVG #1033

notriddle · 2020-09-03T20:47:12Z

Fixes #790

jyn514 · 2020-09-03T21:35:10Z

Can you post some screenshots of what this looks like after the change? I'm a little worried about changing so much of the CSS without any frontend tests ...

notriddle · 2020-09-03T22:15:22Z

If anybody notices a change, other than people who disable web fonts, then I messed up

pietroalbini · 2020-09-04T09:58:38Z

cc @Kixiron

This is great! I'm wondering, could we inline the SVG directly without having to download the .svg file? That would cut a network request completly, and since we don't use icons that much (except for maybe the source listing page?) it should also reduce the amount of data sent to viewers.

pietroalbini · 2020-09-04T09:59:10Z

To be clear, landing this is also fine, if you don't have the time to make the other change or you prefer to do it in a followup :)

notriddle · 2020-09-04T17:38:53Z

Yeah, we probably should switch to inlined SVG. I didn't originally do it, because there's no FontAwesome HOWTO for it, but I've done it before and know how. Also, IE11 shows blank spots where the icons ought to be, and inlining them would fix that.

notriddle · 2020-09-04T21:31:04Z

Okay, it embeds the SVG directly now. It looks identical.

jyn514 · 2020-09-05T03:01:11Z

It looks like you moved all the vendor files into a new repo, which you just published as a crate? https://github.com/notriddle/font-awesome-as-a-crate

What's the rationale for doing that? What's the long-term maintenance plan if we need to update? I want to avoid adding and deleting 8k lines of code every 2 weeks.

Cargo.toml

templates/header/global_alert.html

notriddle · 2020-09-05T19:19:27Z

It looks like you moved all the vendor files into a new repo, which you just published as a crate? https://github.com/notriddle/font-awesome-as-a-crate

Yes.

What's the rationale for doing that?

font-awesome-as-a-crate doesn't just vendor the SVG files. It concatenates them into a massive rust source file.

The rationale for embedding the SVG files directly in your binary:

avoid file I/O occurring in your Tera Filter, which would likely have complex knock-on effects when combined with async
avoid complex error handling in your Tera Filter, which is the inevitable outcome of file I/O
avoid having to carefully construct filesystem paths, with all the terrible possible outcomes and failure modes that come with that (using the infrastructure that the existing static file server uses is not an option, because it's not an HTTP request)
I'm guessing it's faster to pull a string out of the binary's text section than it is to open a file for every request, though I have done no benchmarking to justify this wild speculation

The rationale for doing it in its own crate:

we're not using any of docs.rs's file handling infrastructure anyway, so all the other stuff in its build.rs was just getting in the way while I coded up its codegen script
it should be useful in other Rust web projects, regardless of the web framework they're using
let's not force rustc to parse a 1.3MiB source file every time you change docs.rs; this seems like prime "separate translation unit" material

What's the long-term maintenance plan if we need to update?

I give everyone on the docs.rs team write access to the font-awesome-as-a-crate repository.

But, more generally, I don't actually expect the vendored fontawesome to be updated very often. I know it's happened a couple of times in the last few weeks, but that probably isn't normal. docs.rs used the same fontawesome version for years without upgrading and it was fine. The way it's now being tweaked twice in the same month is an unusual circumstance.

jyn514 · 2020-09-05T19:31:57Z

All your points make sense, except that I'm a little unsure about having font_awesome_as_a_crate in a separate repository, since we're the only ones currently using it. Do you mind making it part of the docs.rs workspace and adding I and @pietroalbini as owners on crates.io? See #1000 for an example of what that would look like.

notriddle · 2020-09-05T19:32:12Z

Sure, let's do that.

vendor/font-awesome-as-a-crate/build.rs

notriddle · 2020-09-05T20:02:17Z

Did you get my invite on crates.io?

jyn514 · 2020-09-05T20:25:35Z

Got it, thanks.

Don't have time to do a full review of this right now but the general idea looks right. Thanks for working on this!

Cargo.toml

vendor/font-awesome-as-a-crate/Cargo.toml

vendor/font-awesome-as-a-crate/build.rs

Unblocks rust-lang#1033.

jyn514

LGTM once #1041 unblocks switching vendor/fontawesome to crates/fontawesome

Unblocks rust-lang#1033.

Unblocks #1033.

jyn514 · 2020-09-14T12:54:29Z

Can you rebase this over master and change the new crate to be in crates/font-awesome-as-a-crate? Looks good to me after that.

Fixes rust-lang#790 The rationale for embedding the SVG files directly in the binary: * avoid file I/O occurring in your Tera Filter, which would likely have complex knock-on effects when combined with `async` * avoid complex error handling in your Tera Filter, which is the inevitable outcome of file I/O * avoid having to carefully construct filesystem paths, with all the terrible possible outcomes and failure modes that come with that (using the infrastructure that the existing static file server uses is not an option, because it's not an HTTP request) * I'm guessing it's faster to pull a string out of the binary's text section than it is to open a file for every request, though I have done no benchmarking to justify this wild speculation The rationale for doing it in its own crate: * we're not using any of docs.rs's file handling infrastructure anyway, so all the other stuff in its build.rs was just getting in the way while I coded up its codegen script * it should be useful in other Rust web projects, regardless of the web framework they're using * let's not force rustc to parse a 1.3MiB source file every time you change docs.rs; this seems like prime "separate translation unit" material

notriddle · 2020-09-14T18:05:54Z

@rustbot modify labels to -S-waiting-on-author, +S-waiting-on-review

rustbot · 2020-09-14T18:05:55Z

Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the master branch to enable it.

Please let @rust-lang/release know if you're having trouble with this bot.

notriddle · 2020-09-14T18:06:07Z

shucks

jyn514 · 2020-09-14T18:13:06Z

I'd take a PR adding triagebot ;)

jyn514 · 2020-09-14T18:52:16Z

Thanks for the PR!

notriddle force-pushed the fontawesome-sprites branch from 9196a2c to cdf6b78 Compare September 3, 2020 20:50

jyn514 added A-frontend Area: Web frontend S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 3, 2020

notriddle force-pushed the fontawesome-sprites branch 2 times, most recently from 29ae43f to 47f2484 Compare September 4, 2020 20:52

notriddle changed the title ~~Switch fontawesome from font files to svg sprites~~ Switch fontawesome from font files to embedded SVG Sep 4, 2020

Kixiron suggested changes Sep 5, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

templates/header/global_alert.html Outdated Show resolved Hide resolved

templates/header/global_alert.html Outdated Show resolved Hide resolved

jyn514 reviewed Sep 5, 2020

View reviewed changes

vendor/font-awesome-as-a-crate/build.rs Outdated Show resolved Hide resolved

vendor/font-awesome-as-a-crate/build.rs Outdated Show resolved Hide resolved

vendor/font-awesome-as-a-crate/build.rs Outdated Show resolved Hide resolved

notriddle force-pushed the fontawesome-sprites branch 5 times, most recently from ba01450 to b70cec5 Compare September 5, 2020 20:01

notriddle force-pushed the fontawesome-sprites branch from ab5fe2f to a62f553 Compare September 6, 2020 04:36

jyn514 reviewed Sep 10, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

vendor/font-awesome-as-a-crate/Cargo.toml Outdated Show resolved Hide resolved

vendor/font-awesome-as-a-crate/Cargo.toml Outdated Show resolved Hide resolved

vendor/font-awesome-as-a-crate/build.rs Outdated Show resolved Hide resolved

jyn514 added a commit to jyn514/docs.rs that referenced this pull request Sep 10, 2020

Rename crates to be in the same top-level crates/ directory

20988b2

Unblocks rust-lang#1033.

jyn514 mentioned this pull request Sep 10, 2020

Rename crates to be in the same top-level crates/ directory #1041

Merged

jyn514 removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Sep 10, 2020

jyn514 approved these changes Sep 10, 2020

View reviewed changes

jyn514 added a commit to jyn514/docs.rs that referenced this pull request Sep 10, 2020

Rename crates to be in the same top-level crates/ directory

f62aac5

Unblocks rust-lang#1033.

jyn514 added a commit that referenced this pull request Sep 14, 2020

Rename crates to be in the same top-level crates/ directory

cc11a32

Unblocks #1033.

jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 14, 2020

notriddle force-pushed the fontawesome-sprites branch from d1cf1c9 to 8d60463 Compare September 14, 2020 17:50

jyn514 added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Sep 14, 2020

jyn514 merged commit f2c5f66 into rust-lang:master Sep 14, 2020

notriddle deleted the fontawesome-sprites branch September 14, 2020 22:28

Switch fontawesome from font files to embedded SVG #1033

Switch fontawesome from font files to embedded SVG #1033

Uh oh!

Conversation

notriddle commented Sep 3, 2020

Uh oh!

jyn514 commented Sep 3, 2020

Uh oh!

notriddle commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

If anybody notices a change, other than people who disable web fonts, then I messed up

Uh oh!

pietroalbini commented Sep 4, 2020

Uh oh!

pietroalbini commented Sep 4, 2020

Uh oh!

notriddle commented Sep 4, 2020

Uh oh!

notriddle commented Sep 4, 2020

Uh oh!

jyn514 commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

notriddle commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jyn514 commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notriddle commented Sep 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

notriddle commented Sep 5, 2020

Uh oh!

jyn514 commented Sep 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

jyn514 commented Sep 14, 2020

Uh oh!

notriddle commented Sep 14, 2020

Uh oh!

rustbot commented Sep 14, 2020

Uh oh!

notriddle commented Sep 14, 2020

Uh oh!

jyn514 commented Sep 14, 2020

Uh oh!

jyn514 commented Sep 14, 2020

Uh oh!

Uh oh!

notriddle commented Sep 3, 2020 •

edited

Loading

jyn514 commented Sep 5, 2020 •

edited

Loading

notriddle commented Sep 5, 2020 •

edited

Loading

jyn514 commented Sep 5, 2020 •

edited

Loading