search

Munter / subfont

Command line tool to optimize your webfont loading. Aggressive subsetting based on your font use, self-hosting of Google fonts and preloading
MIT License
1.56k stars 29 forks source link

Subfont does modify index.html with new assets #160

Closed ScottGrun closed 2 years ago

ScottGrun commented 2 years ago

Hey there, I am trying to use subfont with a ^1.0.0-beta.40 project, I have tried running subfont -i -r --root dist and subfont -i -r --root dist/index.html both manually and as postbuild scripts. It generates all the new subfonts and modifies the CSS but doesn't touch the index.html file so the fonts never get imported and I am left with fallback fonts. Has anyone come across this?

papandreou commented 2 years ago

Could you try with subfont -i -r --root dist dist/index.html?

ScottGrun commented 2 years ago

Still not working, it may just be Astro issue which wouldn't be suprising since it's still in beta so API is changing all the time.

papandreou commented 2 years ago

Hmm, can you share a zip with the contents of the dist folder so I can try to repro?

CxRes commented 2 years ago

I am having the exact same issue. I have all my @fontface declarations in a separate CSS file which is now empty. Instead, there is a file subfont/fonts-[hash].css generated but this is not linked from 'index.html'. In fact, no file is modified and only a folder subfont/ is generated with font and fallback css.

Outputting to another folder also recreates my assets/ folder which contains all the linked files. But no index.html and some an SVG very slightly modified. Strange!

Unfortunately, the project is quite complex to provide for a repro, but I will try on another project!

papandreou commented 2 years ago

Thanks! I hope you can provide a repro 🙏

CxRes commented 2 years ago

Repro as requested

These are reproductions of my personal page https://cxres.pages.dev with manual font optimizations removed (you can check the optimizations in the original).

The original is cf-test-base.

Case 1

I copied this to cf-test-mod and ran: 

subfont --root=./_public/cf-test-mod/ -i -r --no-fallbacks ./_public/cf-test-mod/index.html

In this first case index.html is unmodified.

Case 2

I also ran subfont with a new output folder cf-test-mod-out:

subfont --root=./_public/cf-test-base/ -i -r --no-fallbacks --output=./_public/cf-test-mod-out ./_public/cf-test-base/index.html

As you can see in this folder that there is no index.html in the output.

I have not enabled fallbacks as it was just creating reduced font files in subfont/ folder and everything else was the same.

papandreou commented 2 years ago

Hmm, I tried extracting _public.zip to a directory called _public in my subfont clone, then:

  1. Reformatted _public/cf-test-base/index.html with vscode (just so it's easier to make out which changes subfont is making)
  2. Made _public/cf-test-mod a fresh copy: rm -fr _public/cf-test-mod && cp -R _public/cf-test-base _public/cf-test-mod
  3. Ran your Case 1 command: subfont --root=./_public/cf-test-mod -i -r --no-fallbacks _public/cf-test-mod/index.html

Output:

 ✔ 0.005 secs: logEvents
 ✔ 0.455 secs: loadAssets
 ✔ 0.501 secs: populate
 ✔ 0.016 secs: checkIncompatibleTypes
 ✔ 0.021 secs: applySourceMaps
 ✔ 0.038 secs: populate
 ✔ 0.028 secs: populate
 ℹ INFO: Missing glyph fallback detected.
         When your primary webfont doesn't contain the glyphs you use, browsers that don't support unicode-range will load your fallback fonts, which will be a potential waste of bandwidth.
         These glyphs are used on your site, but they don't exist in the font you applied to them:
         - \u{1f4cb} (📋) in font-family 'Saira Extra Condensed' (500/normal) at _public/cf-test-mod/index.html:56:98
         - \u{1f511} (🔑) in font-family 'Saira Extra Condensed' (500/normal) at _public/cf-test-mod/index.html:59:138
 ✔ 0.067 secs: serializeSourceMaps
 ✔ 0.008 secs: writeAssetsToDisc
_public/cf-test-mod/index.html: 2 fonts (4 variants) in use, 54.7 kB total. Created subsets: 18.5 kB total
  Josefin Sans:
    400 : 70/219 codepoints used, 10.8 kB (woff2) => 5.32 kB (woff2)
    700 : 43/219 codepoints used, 10.5 kB (woff2) => 3.54 kB (woff2)
  Saira Extra Condensed:
    700 : 25/222 codepoints used, 16.7 kB (woff2) => 3.94 kB (woff2)
    500 : 46/222 codepoints used, 16.7 kB (woff2) => 5.69 kB (woff2)
HTML/SVG/JS/CSS size increase: 23.3 kB
Total savings: 12.9 kB
Output written to file:///home/andreas/work/subfont/_public/cf-test-mod/

This makes a bunch of changes to _public/cf-test-mod/index.html, including a <link rel="stylesheet" href="/subfont/fonts-1ba3ed4ddb.css"> being added:

diff --git a/_public/cf-test-mod/index.html b/_public/cf-test-mod/index.html
index 81c9df0..009644d 100644
--- a/_public/cf-test-mod/index.html
+++ b/_public/cf-test-mod/index.html
@@ -1,44 +1,19 @@
-<!DOCTYPE html>
-<html lang="en">
-  <head>
-    <meta charset="utf-8" />
-    <meta
-      name="viewport"
-      content="width=device-width, initial-scale=1, shrink-to-fit=no"
-    />
-    <meta name="description" content="Resume | Rahul Gupta" />
-    <meta name="author" content="Rahul Gupta" />
+<!DOCTYPE html><html lang="en"><head>
+    <meta charset="utf-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
+    <meta name="description" content="Resume | Rahul Gupta">
+    <meta name="author" content="Rahul Gupta">
     <title>Rahul Gupta</title>
-    <script src="/email.29f1cc60.js" type="module" defer></script>
-    <script src="/scripts.1e5907e6.js" type="module" defer></script>
-    <link href="/fonts.75140ba4.css" rel="stylesheet" />
-    <link href="/styles.313dc37d.css" rel="stylesheet" />
-    <link rel="icon" href="/assets/favicon.52dd45b7.png" />
+    <script src="/email.29f1cc60.js" type="module" defer=""></script>
+    <script src="/scripts.1e5907e6.js" type="module" defer=""></script>
+    <link rel="stylesheet" href="/subfont/fonts-1ba3ed4ddb.css"><link href="/fonts.75140ba4.css" rel="stylesheet">
+    <link href="/styles.313dc37d.css" rel="stylesheet">
+    <link rel="icon" href="/assets/favicon.52dd45b7.png">
   </head>
[...]

That also happens if I leave out the pretty-printing step. So I can't really reproduce what you're seeing. Am I overlooking something subtle?

(I do see some weirdness in the output, eg. _public/cf-test-mod/fonts.75140ba4.css ending up empty except for the source map reference, and still being referenced -- should probably take a look at that. Must be due to the Css#isEmpty method not considering it empty because of the /*# sourceMappingURL=... */ :zany_face:)

In the second case I think the problem is that the combination of -i (in-place) and an output dir (--output) doesn't really make sense. We should probably add a guard against that.

papandreou commented 2 years ago

(I do see some weirdness in the output, eg. _public/cf-test-mod/fonts.75140ba4.css ending up empty except for the source map reference, and still being referenced -- should probably take a look at that. Must be due to the Css#isEmpty method not considering it empty because of the /# sourceMappingURL=... / :zany_face:)

Fixed that in 3dfd9dfbbe5071b94eb52ac216796bc7c0554078, at least so the empty stylesheet is no longer referenced from index.html in the output.

CxRes commented 2 years ago

Thanks for looking into it. I am flummoxed too! Could this be an environment issue? Are you doing something platform specific? Is Assetgraph doing something platform specific?

I am on: Node: 16.16.0 npm: 8.15.0 Windows 10 x64 I also use pnpm (but I don't see how that has any bearing)

I reran everything just to double-check (mostly out of disbelief). My output for case 1 (again fresh copy of -base to -mod):

$ pnpm subfont --root=./_public/cf-test-mod/ -i -r --no-fallbacks ./_public/cf-test-mod/index.html
 ✔ 0.007 secs: logEvents
 ✔ 0.663 secs: loadAssets
 ✔ 0.847 secs: populate
 ✔ 0.027 secs: checkIncompatibleTypes
 ✔ 0.046 secs: applySourceMaps
 ✔ 0.347 secs: populate
 ✔ 0.248 secs: populate
 ℹ INFO: Missing glyph fallback detected.
         When your primary webfont doesn't contain the glyphs you use, browsers that don't support unicode-range will load your fallback fonts, which will be a potential waste of bandwidth.
         These glyphs are used on your site, but they don't exist in the font you applied to them:
         - \u{d83d} (📋) in font-family 'Saira Extra Condensed' (500/normal) at _public\cf-test-mod\index.html:1:2738
         - \u{d83d} (🔑) in font-family 'Saira Extra Condensed' (500/normal) at _public\cf-test-mod\index.html:1:2872
 ✔ 0.089 secs: serializeSourceMaps
 ✔ 0.020 secs: writeAssetsToDisc
_public\cf-test-mod\index.html: 2 fonts (4 variants) in use, 54.7 kB total. Created subsets: 18.5 kB total
  Josefin Sans:
    400 : 70/219 codepoints used, 10.8 kB (woff2) => 5.32 kB (woff2)
    700 : 43/219 codepoints used, 10.5 kB (woff2) => 3.58 kB (woff2)
  Saira Extra Condensed:
    700 : 25/222 codepoints used, 16.7 kB (woff2) => 3.92 kB (woff2)
    500 : 46/222 codepoints used, 16.7 kB (woff2) => 5.68 kB (woff2)
HTML/SVG/JS/CSS size increase: 1.12 kB
Total savings: 35.1 kB

Notice the size change is much smaller. Also, there is no difference between -base/index.html and -mod/index.html that I can report.

I also ran this with node 18. No difference in outcome!

papandreou commented 2 years ago

Hmm, there shouldn't be anything environment-specific going on. The code that writes out the modified files is just using the fs module and mkdirp.

I don't have access to a Windows box. Maybe you could try applying this patch and then see if the output makes sense? Ie. do the paths look OK, and do the logged file sizes match the size of index.html etc.

diff --git a/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js b/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js
index 332a518..d3b5bfd 100644
--- a/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js
+++ b/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js
@@ -6,10 +6,13 @@ const urlTools = require('urltools');

 async function mkpathAndWriteFileAsync(fileName, contents, encoding) {
   try {
+    console.log(`Writing ${fileName} (${Buffer.byteLength(contents)} bytes)`);
     await fs.writeFileAsync(fileName, contents, encoding);
   } catch (err) {
     if (err.code === 'ENOENT') {
+      console.log(`Missing containing dir for ${fileName}, creating ${Path.dirname(fileName)}`);
       await mkdirpAsync(Path.dirname(fileName));
+      console.log(`2nd attempt: Writing ${fileName} (${Buffer.byteLength(contents)} bytes)`);
       await fs.writeFileAsync(fileName, contents, encoding);
     } else {
       throw err;
papandreou commented 2 years ago

Could also be interesting to lower the concurrency:

diff --git a/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js b/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js
index 332a518..c904d1e 100644
--- a/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js
+++ b/node_modules/assetgraph/lib/transforms/writeAssetsToDisc.js
@@ -62,7 +62,7 @@ module.exports = (queryObj, outRoot, root) => {
           );
         }
       },
-      { concurrency: 40 }
+      { concurrency: 1 }
     );
   };
 };
CxRes commented 2 years ago

Somehow index.html is missing from the assetgraph itself. Concurrency has no effect!

$  pnpm subfont --root=./_public/cf-test-mod/ -i -r --no-fallbacks ./_public/cf-test-mod/index.html
 ✔ 0.007 secs: logEvents
 ✔ 0.440 secs: loadAssets
 ✔ 0.904 secs: populate
 ✔ 0.017 secs: checkIncompatibleTypes
 ✔ 0.052 secs: applySourceMaps
 ✔ 0.370 secs: populate
 ✔ 0.250 secs: populate
 ℹ INFO: Missing glyph fallback detected.
         When your primary webfont doesn't contain the glyphs you use, browsers that don't support unicode-range will load your fallback fonts, which will be a potential waste of bandwidth.
         These glyphs are used on your site, but they don't exist in the font you applied to them:
         - \u{d83d} (📋) in font-family 'Saira Extra Condensed' (500/normal) at _public\cf-test-mod\index.html:1:2738
         - \u{d83d} (🔑) in font-family 'Saira Extra Condensed' (500/normal) at _public\cf-test-mod\index.html:1:2872
 ✔ 0.042 secs: serializeSourceMaps
Writing D:/_public/cf-test-mod/email.29f1cc60.js (281 bytes)
Writing D:/_public/cf-test-mod/subfont/Saira_Extra_Condensed-500-f911c86d43.woff (7180 bytes)
Missing containing dir for D:/_public/cf-test-mod/subfont/Saira_Extra_Condensed-500-f911c86d43.woff, creating D:/_public/cf-test-mod/subfont
2nd attempt: Writing D:/_public/cf-test-mod/subfont/Saira_Extra_Condensed-500-f911c86d43.woff (7180 bytes)
Writing D:/_public/cf-test-mod/subfont/Saira_Extra_Condensed-500-8066728329.woff2 (5680 bytes)
Writing D:/_public/cf-test-mod/subfont/Saira_Extra_Condensed-700-8b50a93f2b.woff (5060 bytes)
Writing D:/_public/cf-test-mod/subfont/Saira_Extra_Condensed-700-cfc30011da.woff2 (3920 bytes)
Writing D:/_public/cf-test-mod/subfont/Josefin_Sans-700-8dce0c7a6c.woff (4772 bytes)
Writing D:/_public/cf-test-mod/subfont/Josefin_Sans-700-b2a33925e4.woff2 (3580 bytes)
Writing D:/_public/cf-test-mod/subfont/Josefin_Sans-400-4f3b717eeb.woff (6984 bytes)
Writing D:/_public/cf-test-mod/subfont/Josefin_Sans-400-ff5d69b340.woff2 (5316 bytes)
Writing D:/_public/cf-test-mod/subfont/fonts-655dadb3e7.css (1509 bytes)
Writing D:/_public/cf-test-mod/styles.313dc37d.css.map (11802 bytes)
Writing D:/_public/cf-test-mod/assets/person-military-pointing.b8c9af17.svg (845 bytes)
Writing D:/_public/cf-test-mod/assets/hand-point-up.bc40232e.svg (1040 bytes)
Writing D:/_public/cf-test-mod/assets/people-group.1c824726.svg (1380 bytes)
Writing D:/_public/cf-test-mod/assets/crown.fdc31a02.svg (832 bytes)
Writing D:/_public/cf-test-mod/assets/bullhorn.4ada893d.svg (744 bytes)
Writing D:/_public/cf-test-mod/assets/scroll.05f16896.svg (598 bytes)
Writing D:/_public/cf-test-mod/assets/book.634b118c.svg (713 bytes)
Writing D:/_public/cf-test-mod/assets/certificate.ebac052b.svg (1207 bytes)
Writing D:/_public/cf-test-mod/assets/flag-checkered.3bb36915.svg (1548 bytes)
Writing D:/_public/cf-test-mod/assets/graduation-cap.596ed9b7.svg (965 bytes)
Writing D:/_public/cf-test-mod/assets/trophy.3409c127.svg (1132 bytes)
Writing D:/_public/cf-test-mod/fonts.75140ba4.css.map (345 bytes)
Writing D:/_public/cf-test-mod/assets/by-nd.ec7f5109.svg (5920 bytes)
Writing D:/_public/cf-test-mod/LICENSE (18829 bytes)
Writing D:/_public/cf-test-mod/NOTICE (783 bytes)
Writing D:/_public/cf-test-mod/docs/RahulGupta-OpenAutomationFramework.pdf (0 bytes)
Writing D:/_public/cf-test-mod/keys/publickey.8bafa14b1a6a2f7d8912ea197e769bbd430c2179.asc (0 bytes)
Writing D:/_public/cf-test-mod/assets/profile.262c56fa.jpg (11382 bytes)
Writing D:/_public/cf-test-mod/assets/favicon.52dd45b7.png (15972 bytes)
Writing D:/_public/cf-test-mod/styles.313dc37d.css (19782 bytes)
Writing D:/_public/cf-test-mod/fonts.75140ba4.css (47 bytes)
Writing D:/_public/cf-test-mod/scripts.1e5907e6.js (63576 bytes)
 ✔ 0.190 secs: writeAssetsToDisc
_public\cf-test-mod\index.html: 2 fonts (4 variants) in use, 54.7 kB total. Created subsets: 18.5 kB total
  Josefin Sans:
    400 : 70/219 codepoints used, 10.8 kB (woff2) => 5.32 kB (woff2)
    700 : 43/219 codepoints used, 10.5 kB (woff2) => 3.58 kB (woff2)
  Saira Extra Condensed:
    700 : 25/222 codepoints used, 16.7 kB (woff2) => 3.92 kB (woff2)
    500 : 46/222 codepoints used, 16.7 kB (woff2) => 5.68 kB (woff2)
HTML/SVG/JS/CSS size increase: 1.12 kB
Total savings: 35.1 kB
Output written to file:///D:/_public/cf-test-mod/
CxRes commented 2 years ago

May I also suggest that you ship a built version of Subfont using WebPack/Rollup/ESBuild. This is generally a good practice nowadays (despite the promises of npm) as it can in many cases help avoid subtle bugs that can occur due to difference in state between different computers, in particular, npm installs. In the case of Subfont, I find there is not even a lockfile in the npm package.

If you are reluctant to publish it as subfont (using exports property in package.json), you could publish it as subfont-prebuilt or similar ie as a separate npm project.

papandreou commented 2 years ago

@CxRes, hmm, index.html must be in the graph (or it wouldn't be able to trace anything characters), but I wonder if it gets skipped when writing out the files because of this criterion: https://github.com/Munter/subfont/blob/3dfd9dfbbe5071b94eb52ac216796bc7c0554078/lib/subfont.js#L299

Mind applying this patch and trying again?

diff --git a/lib/subfont.js b/lib/subfont.js
index 476872d..c7ad0c5 100644
--- a/lib/subfont.js
+++ b/lib/subfont.js
@@ -296,7 +296,14 @@ module.exports = async function subfont(
       {
         isLoaded: true,
         isRedirect: { $ne: true },
-        url: (url) => url.startsWith(assetGraph.root),
+        url: (url) => {
+          console.log(
+            `Does ${url} start with ${assetGraph.root}? ${url.startsWith(
+              assetGraph.root
+            )}`
+          );
+          return url.startsWith(assetGraph.root);
+        },
       },
       outRoot,
       assetGraph.root
papandreou commented 2 years ago

May I also suggest that you ship a built version of Subfont using WebPack/Rollup/ESBuild. This is generally a good practice nowadays (despite the promises of npm) as it can in many cases help avoid subtle bugs that can occur due to difference in state between different computers, in particular, npm installs. In the case of Subfont, I find there is not even a lockfile in the npm package.

Sounds like a subject for another issue. It would be a big task that would involve splitting the current subfont up into a bunch of pieces -- which would probably be a good idea no matter what.

CxRes commented 2 years ago

This is it, it is indeed a Windows specific bug:

(quoting relevant portion of the output only only):

...
Does file:///D:/_public/cf-test-mod/index.html start with file:///D%3A/_public/cf-test-mod/? false
Does file:///D%3A/_public/cf-test-mod/email.29f1cc60.js start with file:///D%3A/_public/cf-test-mod/? true 
...

Looks like Assetgraph does not convert the D: for the entry file to D%3A, but does so for all other assets, causing the test to fail.

papandreou commented 2 years ago

Ah, that should be fixable, I can take a look tonight.

papandreou commented 2 years ago

I just published subfont 6.8.0, which I hope will fix it. Could you try upgrading to that?

CxRes commented 2 years ago

LGTM. Thanks for the fix!!!

Can I catch you on DM somewhere? I have one or two short questions, basically relating to how I can run Assetgraph-builder, Seespee and Subfont together as one tool without writing multiple times to disk.

Sounds like a subject for another issue. It would be a big task that would involve splitting the current subfont up into a bunch of pieces -- which would probably be a good idea no matter what.

I don't follow why you need to break things up for this? What I meant was creating a single bundle/file in which the dependencies have been included!

Do you want me to open another issue for it?

papandreou commented 2 years ago

Can I catch you on DM somewhere? I have one or two short questions, basically relating to how I can run Assetgraph-builder, Seespee and Subfont together as one tool without writing multiple times to disk.

There should be room to talk about that here. It's easier to involve others when it's in a public forum. But otherwise you can shoot me an email.

Sounds like a subject for another issue. It would be a big task that would involve splitting the current subfont up into a bunch of pieces -- which would probably be a good idea no matter what.

I don't follow why you need to break things up for this? What I meant was creating a single bundle/file in which the dependencies have been included!

Do you want me to open another issue for it?

Ah, I misunderstood you the first time, I thought you meant turning subfont into a plugin for said build tools. Yeah, please open a new issue, then we can figure out if it's worth it.

CxRes commented 2 years ago

There should be room to talk about that here. It's easier to involve others when it's in a public forum. But otherwise you can shoot me an email.

I am reluctant to do that here because I also do not like to mix conversations. But in the spirit of your request I'll open another issue in Assetgraph-builder marking it as a discussion.

I am unable to close this issue as I did not start it. You would need to do that.