Remove support for subsetPerPage:true (semver-major)

Munter · Aug 8, 2020 · 90f109c · 90f109c
1 parent 7609a24
commit 90f109c
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 509 deletions.
diff --git a/README.md b/README.md
@@ -91,8 +91,6 @@ Options:
                                                                         [boolean] [default: false]
   --font-display                     Injects a font-display value into the @font-face CSS.
              [string] [choices: "auto", "block", "swap", "fallback", "optional"] [default: "swap"]
-  --subset-per-page                  Create a unique subset for each page.
-                                                                        [boolean] [default: false]
   --recursive, -r                    Crawl all HTML-pages linked with relative and root relative
                                      links. This stays inside your domain
                                                                         [boolean] [default: false]

diff --git a/lib/parseCommandLineOptions.js b/lib/parseCommandLineOptions.js
@@ -72,11 +72,6 @@ module.exports = function parseCommandLineOptions(argv) {
       default: 'swap',
       choices: ['auto', 'block', 'swap', 'fallback', 'optional'],
     })
-    .options('subset-per-page', {
-      describe: 'Create a unique subset for each page.',
-      type: 'boolean',
-      default: false,
-    })
     .options('recursive', {
       alias: 'r',
       describe:
@@ -108,7 +103,13 @@ module.exports = function parseCommandLineOptions(argv) {
       type: 'boolean',
       default: false,
     })
-    .check(({ harfbuzz }) => {
+    .check(({ harfbuzz, subsetPerPage }) => {
+      // Fail instead of silently ignoring legacy switches.
+      // This prevents --subset-per-page etc. from implicitly being interpreted as 'string',
+      // which means that it would consume a following non-option cli param
+      if (subsetPerPage !== undefined) {
+        return '--[no-]subset-per-page is no longer supported as of subfont 6.0.0';
+      }
       if (harfbuzz && parseInt(process.versions.node) < 10) {
         return 'The --harfbuzz option requires node.js 10 or above';
       } else {

diff --git a/lib/subfont.js b/lib/subfont.js
@@ -18,7 +18,6 @@ module.exports = async function subfont(
     inlineCss = false,
     fontDisplay = 'swap',
     formats,
-    subsetPerPage = false,
     inPlace = false,
     inputFiles = [],
     recursive = false,
@@ -219,7 +218,6 @@ module.exports = async function subfont(
     inlineFonts,
     inlineCss,
     fontDisplay,
-    subsetPerPage,
     formats,
     jsPreload,
     omitFallbacks: !fallbacks,
@@ -364,7 +362,6 @@ module.exports = async function subfont(
           fontUsage.codepoints.original.length
         ).padStart(String(maxOriginalCodePoints).length)} codepoints used`;
         if (
-          !subsetPerPage &&
           fontUsage.codepoints.page.length !== fontUsage.codepoints.used.length
         ) {
           status += ` (${fontUsage.codepoints.page.length} on this page)`;

diff --git a/lib/subsetFonts.js b/lib/subsetFonts.js
@@ -104,12 +104,12 @@ function getFontFaceDeclarationText(node, relations) {
 // Takes the output of fontTracer
 function groupTextsByFontFamilyProps(
   htmlAsset,
-  textByPropsArray,
-  availableFontFaceDeclarations,
-  isOnPageFn
+  globalTextByPropsArray,
+  pageTextByPropsArray,
+  availableFontFaceDeclarations
 ) {
-  const snappedTexts = _.flatMapDeep(textByPropsArray, (textAndProps) => {
-    const isOnPage = isOnPageFn(textAndProps);
+  const snappedTexts = _.flatMapDeep(globalTextByPropsArray, (textAndProps) => {
+    const isOnPage = pageTextByPropsArray.includes(textAndProps);
     const family = textAndProps.props['font-family'];
     if (family === undefined) {
       return [];
@@ -649,7 +649,6 @@ async function subsetFonts(
     subsetPath = 'subfont/',
     jsPreload = true,
     omitFallbacks = false,
-    subsetPerPage,
     inlineFonts,
     inlineCss,
     fontDisplay,
@@ -811,9 +810,9 @@ async function subsetFonts(
     } = htmlAssetTextsWithPropsEntry;
     htmlAssetTextsWithPropsEntry.fontUsages = groupTextsByFontFamilyProps(
       htmlAsset,
-      subsetPerPage ? textByProps : globalTextByProps,
-      accumulatedFontFaceDeclarations,
-      (t) => (subsetPerPage ? true : textByProps.includes(t))
+      globalTextByProps,
+      textByProps,
+      accumulatedFontFaceDeclarations
     );
   }
 

diff --git a/test/subfont.js b/test/subfont.js
@@ -432,98 +432,56 @@ describe('subfont', function () {
     );
   });
 
-  describe('when subsetPerPage is false', function () {
-    it('should report how many codepoints are used on the page as well as globally', async function () {
-      const root = encodeURI(
-        `file://${pathModule.resolve(
-          __dirname,
-          '..',
-          'testdata',
-          'differentCodepointsOnDifferentPages'
-        )}`
-      );
-
-      await subfont(
-        {
-          subsetPerPage: false,
-          silent: true,
-          dryRun: true,
-          root,
-          inputFiles: [`${root}/first.html`, `${root}/second.html`],
-        },
-        mockConsole
-      );
-      expect(mockConsole.log, 'to have a call satisfying', () => {
-        mockConsole.log(
-          expect.it(
-            'to contain',
-            '400 : 6/214 codepoints used (3 on this page),'
-          )
-        );
-      }).and('to have a call satisfying', () => {
-        mockConsole.log(
-          expect.it(
-            'to contain',
-            '400 : 6/214 codepoints used (4 on this page),'
-          )
-        );
-      });
-    });
+  it('should report how many codepoints are used on the page as well as globally', async function () {
+    const root = encodeURI(
+      `file://${pathModule.resolve(
+        __dirname,
+        '..',
+        'testdata',
+        'differentCodepointsOnDifferentPages'
+      )}`
+    );
 
-    // Regression test for https://gitter.im/assetgraph/assetgraph?at=5f1ddc1afe6ecd2888764496
-    it('should not crash in the reporting code when a font has no text on a given page', async function () {
-      const root = encodeURI(
-        `file://${pathModule.resolve(
-          __dirname,
-          '..',
-          'testdata',
-          'noFontUsageOnOnePage'
-        )}`
+    await subfont(
+      {
+        silent: true,
+        dryRun: true,
+        root,
+        inputFiles: [`${root}/first.html`, `${root}/second.html`],
+      },
+      mockConsole
+    );
+    expect(mockConsole.log, 'to have a call satisfying', () => {
+      mockConsole.log(
+        expect.it('to contain', '400 : 6/214 codepoints used (3 on this page),')
       );
-
-      await subfont(
-        {
-          silent: true,
-          dryRun: true,
-          root,
-          inputFiles: [`${root}/first.html`, `${root}/second.html`],
-        },
-        mockConsole
+    }).and('to have a call satisfying', () => {
+      mockConsole.log(
+        expect.it('to contain', '400 : 6/214 codepoints used (4 on this page),')
       );
     });
   });
 
-  describe('when subsetPerPage is true', function () {
-    it('should report how many codepoints are used on the page as well as globally', async function () {
-      const root = encodeURI(
-        `file://${pathModule.resolve(
-          __dirname,
-          '..',
-          'testdata',
-          'differentCodepointsOnDifferentPages'
-        )}`
-      );
+  // Regression test for https://gitter.im/assetgraph/assetgraph?at=5f1ddc1afe6ecd2888764496
+  it('should not crash in the reporting code when a font has no text on a given page', async function () {
+    const root = encodeURI(
+      `file://${pathModule.resolve(
+        __dirname,
+        '..',
+        'testdata',
+        'noFontUsageOnOnePage'
+      )}`
+    );
 
-      await subfont(
-        {
-          subsetPerPage: true,
-          silent: true,
-          dryRun: true,
-          root,
-          inputFiles: [`${root}/first.html`, `${root}/second.html`],
-        },
-        mockConsole
-      );
-      expect(mockConsole.log, 'to have a call satisfying', () => {
-        mockConsole.log(
-          expect.it('to contain', '400 : 3/214 codepoints used,')
-        );
-      }).and('to have a call satisfying', () => {
-        mockConsole.log(
-          expect.it('to contain', '400 : 4/214 codepoints used,')
-        );
-      });
-    });
+    await subfont(
+      {
+        silent: true,
+        dryRun: true,
+        root,
+        inputFiles: [`${root}/first.html`, `${root}/second.html`],
+      },
+      mockConsole
+    );
   });
 
   describe('with --dynamic', function () {