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

Bump @cubing/icons to v3.0.0 #10526

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Bump @cubing/icons to v3.0.0 #10526

wants to merge 5 commits into from

Conversation

lgarron
Copy link
Member

@lgarron lgarron commented Jan 4, 2025

This should be a compatible replacement. I've done a basic check that the icons show up properly, and a full switch on the WCA website will help shake out potential issues.

@lgarron lgarron force-pushed the main branch 2 times, most recently from 2d4f0fa to 67c363f Compare January 4, 2025 01:15
@lgarron
Copy link
Member Author

lgarron commented Jan 4, 2025

I also can't figure out what's going on here:

@import "cubing-icons"; /* stylelint-disable-line no-invalid-position-at-import-rule */

The string cubing-icons doesn't appear anywhere else in the repo, so I have no idea where that's pointing. 😳

This should be a compatible replacement. I've done a basic check that the icons show up properly, and a full switch on the WCA website will help shake out potential issues.
@lgarron
Copy link
Member Author

lgarron commented Jan 4, 2025

SASS is now erroring in CI without printing an error (it's just printing warnings). No idea what's going on with that. 🤷

@gregorbg
Copy link
Member

gregorbg commented Jan 4, 2025

SASS is now erroring in CI without printing an error (it's just printing warnings). No idea what's going on with that. 🤷

I'm getting something that looks very much like an error:
image

This is related to the stylesheets for the "print competition information as PDF" feature, which unfortunately predates every active member on WST, myself included.

@jonatanklosko might know more about this, or he might not. Otherwise, @viroulep is probably the person to talk to.

@jonatanklosko
Copy link
Member

No idea, but the package was already @cubing/icons before and the import worked, so perhaps it would work as it was?

@lgarron
Copy link
Member Author

lgarron commented Jan 4, 2025

I'm getting something that looks very much like an error:

Ah, somehow I missed that? Anyhow, looks like cubing-icons and @cubing/icons both error in that location now. I don't see how the former should ever have worked, so I'm still not sure what to do.

@viroulep
Copy link
Contributor

viroulep commented Jan 6, 2025

You can find the file by looking at the repository in its state when I introduced the pdf export: https://github.com/thewca/worldcubeassociation.org/blob/c15867cfe662906f65fe7d90c74a636c04cbc470/WcaOnRails/vendor/assets/stylesheets/cubing-icons.css

I have no idea what happened to this file though.

@lgarron
Copy link
Member Author

lgarron commented Jan 6, 2025

You can find the file by looking at the repository in its state when I introduced the pdf export: https://github.com/thewca/worldcubeassociation.org/blob/c15867cfe662906f65fe7d90c74a636c04cbc470/WcaOnRails/vendor/assets/stylesheets/cubing-icons.css

I have no idea what happened to this file though.

Yeah, thins would make a lot more sense if this file still existed. Now that's no longer in the tree, I don't know how it's still load-bearing. 😳

@lgarron
Copy link
Member Author

lgarron commented Jan 6, 2025

Aha!

Might be this:

# Add additional assets to the asset load path
# This is used exclusively to make cubing icons available for the wicked_pdf
# helper!
# wicked_pdf's helpers can't handle "packs_with_chunks" and therefore cannot
# properly inline our packs :(
Rails.application.config.assets.paths << Rails.root.join("node_modules", "@cubing", "icons", "www", "css")

That looks cursed, let me see if I can replace that with anything significantly less hacky.

@gregorbg
Copy link
Member

gregorbg commented Jan 6, 2025

@lgarron
Copy link
Member Author

lgarron commented Jan 9, 2025

@jfly poked me about this, so let me describe my understanding of the situation.

As far as I can tell, we have two problems stemming from the same line:

I also can't figure out what's going on here:

@import "cubing-icons"; /* stylelint-disable-line no-invalid-position-at-import-rule */

The string cubing-icons doesn't appear anywhere else in the repo, so I have no idea where that's pointing. 😳

  1. This line depends breaking package abstraction (e.g. violating the semantics of conditional exports) by adding the CSS file from the package explicitly as an asset here. This makes me grumbly, but it's easy to implement a hack that works.
  2. The code assumes that it can load a single CSS file. @cubing/icons no longer publishes CSS with inline fonts, as this is an anti-pattern.

For the second point, I think the options are:

  • Implement module resolution for the PDF code. Probably prohibitive for now.
  • Write some sort of build step to inline the font in the CSS so it looks the same to the PDF generation code. This is trivial with something like https://esbuild.github.io/content-types/#data-url , but we don't use esbuild and I don't know where in the build pipeline to put it.
  • Implement a compromise that explicitly loads the .woff2 font for the PDF at a path that the CSS can consume.

@lgarron lgarron changed the title Bump @cubing/icons to v2.1.0-pre.2 Bump @cubing/icons to v2.1.0-pre.3 Jan 9, 2025
@lgarron
Copy link
Member Author

lgarron commented Jan 9, 2025

  • Write some sort of build step to inline the font in the CSS so it looks the same to the PDF generation code. This is trivial with something like https://esbuild.github.io/content-types/#data-url , but we don't use esbuild and I don't know where in the build pipeline to put it.

Specifically:

mkdir temp && cd temp
npm install esbuild @cubing/icons
npx esbuild --bundle "@cubing/icons/css" --loader:.woff2=base64 > cubing-icons-with-font-inlined.css

Or in JS/TS:

import { build } from "esbuild";

await build({
  bundle: true,
  entryPoints: ["@cubing/icons/css"],
  loader: {".woff2": "base64"}
})

@lgarron lgarron changed the title Bump @cubing/icons to v2.1.0-pre.3 Bump @cubing/icons to v3.0.0 Jan 9, 2025
@lgarron
Copy link
Member Author

lgarron commented Jan 9, 2025

I strongly disagree with this linter error:

Errors on modified lines:
/home/runner/work/worldcubeassociation.org/worldcubeassociation.org/app/assets/stylesheets/pdf.css.scss: line 11, col 1, error - Unexpected empty comment (scss/comment-no-empty)

I think that satisfying the linter here makes the comments less readable. But I'll smash the comment parts together to make it happy for now.

@lgarron
Copy link
Member Author

lgarron commented Jan 9, 2025

Okay, I tried to update this in a commit. Let's see if it passes CI.

  • The code assumes that it can load a single CSS file. @cubing/icons no longer publishes CSS with inline fonts, as this is an anti-pattern.

It might be that relative path resolution of the font file actually works correctly here. Unfortunately, my codespace checkout seems to be broken for main due to a Shakapacker::Manifest::MissingEntryError, so I can't really test this. @gregorbg, assuming I can get it working, do you know the easiest way to generate a PDF to check if the icons show up properly?

@lgarron
Copy link
Member Author

lgarron commented Jan 9, 2025

Okay, I tried to update this in a commit. Let's see if it passes CI.

🥳

I was able to get local dev working by switching to an older commit. Currently trying to figure out how to get a PDF to display.

@lgarron
Copy link
Member Author

lgarron commented Jan 10, 2025

So, with some difficulty I've been able to test how this affects PDF.

It seems that the icons don't render even if the font is inlined. This is tricky to debug, as I can't actually load the relevant HTML for the PDF easily.

@viroulep, is there a way for me to serve the HTML that is used to render the PDF, so I can access it in a browser?

@lgarron
Copy link
Member Author

lgarron commented Jan 10, 2025

Okay, with a lot of pain I'm able to make slow progress debugging. My current assessment is that this is not working because:

  1. We're rendering PDFs wkhtmltopdf, which is a dead project: https://github.com/wkhtmltopdf/wkhtmltopdf
  2. The rendering engine in wkhtmltopdf is either out of date or is not configured to render WOFF2.
  3. The setup does not work without fonts inlined into the CSS.

To give an idea of how out of date that is: WebKit introduced WOFF2 support back in 2016, somewhere 8 or 9 years ago.
(Some sources imply it might not have been available on all platforms until 2018, but that's still a long time ago.)

However, the PDF does seem to work with WOFF and inlined fonts.

I'm thinking of introducing a build in @cubing/icons/css/legacy-inline-woff that provides this, with the hope that the WCA website will not need to rely on it indefinitely. (After all, relying on wkhtmltopdf indefinitely will at some point become a fairly serious security concern. And we can also hopefully clean up a bunch of the bundling if development can move from webpack to esbuild.)

@gregorbg, how does that sound to you?

@gregorbg
Copy link
Member

Well as far as I can tell this "competition info PDF" is the only place where we ever rely on any server-side PDF generation library. And so it should be fairly easy ™️ to move away from wkhtmltopdf, which I would be happy to get done sooner rather than later.

What do cool kids use these days for PDF generation? Is there any library or backend that would make your life easier in terms of using the fonts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants