Skip to content

Conversation

gene1wood
Copy link
Collaborator

@gene1wood gene1wood commented Dec 29, 2023

With this change, the static assets like fonts are hosted at the root of the path instead of within their subdirectories like /fonts

I've been unable to figure out how to get this to work correctly where the SASS url references to fonts would point to /fonts. To work around this, I've commented out the outputPath in the asset/resource copying over of the fonts.

@april Can you tell at all how to get the URL paths to work correctly?

To see the issue, checkout this PR, npm install and npm run watch and you'll see that
the resulting url paths to the font files are the root of the site instead of /fonts

With this change, the static assets like fonts are hosted at the root of the path
instead of within their subdirectories like `/fonts`

I've been unable to figure out how to get this to work correctly where
the SASS `url` references to fonts would point to `/fonts`. To work around
this, I've commented out the `outputPath` in the `asset/resource` copying over
of the fonts.
@gene1wood
Copy link
Collaborator Author

@april It's gotta be related to the SASS processing. I can control where the files end up in the built directory just fine, but I've been unable to get the SASS loaders to consume the fonts scss file and render url CSS statements with a path present.

@april
Copy link
Contributor

april commented Jan 2, 2024

Sorry, I still haven't had any time to even look at this. Is it consuming the other CSS files fine, or is only the fonts one that isn't working?

@gene1wood
Copy link
Collaborator Author

@april The only uses of url() in the scss files are the fonts. I did try as a test just taking the content of the _fonts.scss file and adding it directly into index.scss in case the @import of _fonts.scss was the issue, but that didn't fix it.

I suspect, if there were any uses of url() elsewhere in the scss files, they too would assume that the referenced asset was in the root.

And in the current PR I have there, the fonts work, because I just said to put the font files in the root where the rendered CSS files reference them. So I could just merge this and it would work, it's just that the fonts would be in the root path instead of the /fonts/ directory (which would be a bit messy but not the end of the world)

@april
Copy link
Contributor

april commented Jan 2, 2024

Does url() work if you set it to a static path rather than a relative one?

Copy link
Collaborator

@janbrasna janbrasna left a comment

Choose a reason for hiding this comment

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

The biggest change is moving away from deprecated node-sass (based on libsass) to sass (that is dart-sass IIRC), and I remember running into filename vs. URI path woes myself during similar transition.

You might need to change url('../fonts/* to url('./fonts/* or experiment with ?url=false in the loader (depending on sass implementation library or webpack+plugins versions you might need switching from "relative to sass file" to "relative to output css file" or "relative to entrypoint" location), the behavior differs if the resulting file is being passed to a loader or not:

Also css-loader changed url() processing between v5 and v6:


Are the fonts needed anyways?

Do you need to self-host the fonts, being Open Sans and Zilla Slab, readily available from e. g. Google Fonts? (Or if you're against hotlinking GFonts there's a Mozilla CDN version: https://github.com/mozilla/zilla-slab#download …)


Some unrelated bits follow:

@@ -17,7 +17,6 @@
<meta name="author" content="<%= htmlWebpackPlugin.options.constants.author %>">
<meta name="description" content="<%= htmlWebpackPlugin.options.constants.description %>">
<meta property="og:title" content="<%= htmlWebpackPlugin.options.constants.title %>">
<link rel="icon" type="image/png" href="/images/favicon.png">
Copy link
Collaborator

Choose a reason for hiding this comment

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

The html-webpack-plugin used to inject the favicon element only if there's filename specified besides template so this might not work out-of-the-box like this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. Ya, it appears to be working.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems that has been fixed then, yay!

Good job updating to Webpack 5.x 🍾 — that unblocked CI options now with node16+ compatibility. (At least for #161 now that GH dropped support for older runners…) — I originally tried bumping stuff to latest 4.47.x that had crypto patched to support node16+ but this gets rid of way more legacy deps so definitely huge thanks for bringing it more up to date!

I have some loader updates to test and will propose them, and one major bump that will need node18+ eventually so that's TBD.

In the meantime I'll check compatibility with Node versions 16–18–20/LTS so we know if there are any limitations ahead of us if we want to drop EOL support and move on…

Once again, amazing job! Thanks 🎉

Comment on lines 165 to 168
test: /\.(svg)$/,
use: {
loader: 'file-loader',
options: {
name: 'img/[name].[ext]'
}
type: 'asset/resource',
generator: {
outputPath: 'img/',
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI there's no *.svg in the output and no assets are currently being built to /img from root so this might be left out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ya, good point. I'll fix that. Thank you.

@gene1wood
Copy link
Collaborator Author

@janbrasna Thanks so much! I dug a bit further based on your guidance but wasn't able to figure it out, but your suggestion of just using hosted fonts sounds great to me, so I'm going to do that. Thanks for your help!

@april
Copy link
Contributor

april commented Jan 3, 2024

Don't forget to add the font-src into the CSP at the top of the page, in the meta tags.

@gene1wood gene1wood merged commit e1a3177 into mozilla:master Jan 3, 2024
@gene1wood gene1wood deleted the webpack5 branch January 3, 2024 18:45
@gene1wood
Copy link
Collaborator Author

I forgot CSP blocks loading of fonts from the CDN. I'll fix that.

@janbrasna
Copy link
Collaborator

Actually you need style-src to @import from https://code.cdn.mozilla.net/fonts/zilla-slab.css

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants