transform <img> to <picture> (github#34429)

lib/render-content/create-processor.js

@@ -19,6 +19,7 @@ import remarkCodeExtra from 'remark-code-extra'
 import codeHeader from './plugins/code-header.js'
 import rewriteLocalLinks from './plugins/rewrite-local-links.js'
 import rewriteImgSources from './plugins/rewrite-asset-urls.js'
+import rewriteAssetImgTags from './plugins/rewrite-asset-img-tags.js'
 import useEnglishHeadings from './plugins/use-english-headings.js'
 import wrapInElement from './plugins/wrap-in-element.js'
 import doctocatLinkIcon from './doctocat-link-icon.js'
@@ -44,6 +45,7 @@ export default function createProcessor(context) {
     .use(raw)
     .use(wrapInElement, { selector: 'ol > li img', wrapper: 'span.procedural-image-wrapper' })
     .use(rewriteImgSources)
+    .use(rewriteAssetImgTags)
     .use(rewriteLocalLinks, context)
     .use(html)
 }

lib/render-content/plugins/rewrite-asset-img-tags.js

@@ -0,0 +1,125 @@
+import { visit, SKIP } from 'unist-util-visit'
+
+/**
+ * `structuredClone` was added in Node 17 and onwards.
+ * https://developer.mozilla.org/en-US/docs/Web/API/structuredClone#browser_compatibility
+ *
+ * At the time of writing, we use Node 18 in CI and in production, but
+ * someone might be previewing locally with an older version so
+ * let's make a quick polyfill.
+ * We could add a specific (`js-core`) package for this polyfill, but it's
+ * fortunately not necessary in this context because it's safe enough
+ * clone by turning into a string and back again.
+ */
+function structuredClonePolyfill(obj) {
+  if (typeof structuredClone !== 'undefined') {
+    return structuredClone(obj)
+  } else {
+    // Note, that this naive clone would turn Date objects into strings.
+    // So don't use this polyfill if certain values aren't primitives
+    // that JSON.parse can handle.
+    return JSON.parse(JSON.stringify(obj))
+  }
+}
+
+const SUPPORT_AVIF_ASSETS = Boolean(JSON.parse(process.env.SUPPORT_AVIF_ASSETS || 'false'))
+
+// This number must match a width we're willing to accept in a dynamic
+// asset URL.
+const MAX_WIDTH = 1000
+
+// Matches any <img> tags with an href that starts with `/assets/`
+const matcher = (node) =>
+  node.type === 'element' &&
+  node.tagName === 'img' &&
+  node.properties &&
+  node.properties.src &&
+  node.properties.src.startsWith('/assets/')
+
+/**
+ * Where it can mutate the AST to swap from:
+ *
+ *   <img src="/assets/help.png" alt="Alternative text">
+ *
+ * To:
+ *
+ *   <picture>
+ *     <source srcset="/assets/help.web" format="image/webp">
+ *     [<source srcset="/assets/help.avif" format="image/avif">]
+ *     <img src="/assets/help.png" alt="Alternative text">
+ *   </picture>
+ *
+ * Note that the AVIF format is optional as it depends on the, off by
+ * default, `process.env.SUPPORT_AVIF_ASSETS`.
+ * */
+export default function rewriteAssetImgTags() {
+  return (tree) => {
+    visit(tree, matcher, (node) => {
+      if (node.properties.src.endsWith('.png')) {
+        const copyPNG = structuredClonePolyfill(node)
+
+        /**
+         * If AVIF is support, we consider it "better" by injecting it first.
+         * If the user agent supports both WebP and AVIF and it gets
+         * HTML that looks like this:
+         *
+         *     <picture>
+         *       <sources srcset="/assets/foo.avif" type="image/avif">
+         *       <sources srcset="/assets/foo.webp" type="image/webp">
+         *       <img src="/assets/foo.png" alt="Alt text">
+         *     </picture>
+         *
+         * Then, browsers like Chrome will try the AVIF format first.
+         * Other browsers, that don't support AVIF will use WebP.
+         *  */
+        if (SUPPORT_AVIF_ASSETS) {
+          const sourceAVIF = {
+            type: 'element',
+            tagName: 'source',
+            properties: {
+              srcset: injectMaxWidth(node.properties.src.replace(/\.png$/, '.avif'), MAX_WIDTH),
+              type: 'image/avif',
+            },
+            children: [],
+          }
+          node.children.push(sourceAVIF)
+        }
+
+        const sourceWEBP = {
+          type: 'element',
+          tagName: 'source',
+          properties: {
+            srcset: injectMaxWidth(node.properties.src.replace(/\.png$/, '.webp'), MAX_WIDTH),
+            type: 'image/webp',
+          },
+          children: [],
+        }
+        node.children.push(sourceWEBP)
+
+        node.children.push(copyPNG)
+        node.tagName = 'picture'
+        delete node.properties.alt
+        delete node.properties.src
+        // Don't go further or else you end up in an infinite recursion
+        return SKIP
+      }
+    })
+  }
+}
+
+/**
+ * Given a pathname, insert the `/_mw-DDDD/`.
+ *
+ * For example, if the pathname is `/assets/cb-1234/images/foo.png`
+ * return `/assets/cb-1234/_mw-1000/images/foo.png`
+ */
+function injectMaxWidth(pathname, maxWidth) {
+  const split = pathname.split('/')
+  // This prefix needs to match what's possibly expected in dynamic-assets.js
+  const inject = `mw-${maxWidth}`
+  if (split.includes(inject)) {
+    throw new Error(`pathname already includes '${inject}'`)
+  }
+  split.splice(3, 0, inject)
+  return split.join('/')
+}

middleware/dynamic-assets.js

@@ -5,6 +5,35 @@ import sharp from 'sharp'
 import { assetCacheControl, defaultCacheControl } from './cache-control.js'
 import { setFastlySurrogateKey, SURROGATE_ENUMS } from './set-fastly-surrogate-key.js'
 
+/**
+ * This is the indicator that is a virtual part of the URL.
+ * Similar to `/cb-1234/` in asset URLs, it's just there to tell the
+ * middleware that the image can be aggressively cached. It's not
+ * part of the actual file-on-disk path.
+ * Similarly, `/mw-1000/` is virtual and will be observed and removed from
+ * the pathname before trying to look it up as disk-on-file.
+ * The exact pattern needs to match how it's set in whatever Markdown
+ * processing code that might make dynamic asset URLs.
+ * So if you change this, make sure you change the code that expects
+ * to be able to inject this into the URL.
+ */
+const maxWidthPathPartRegex = /\/mw-(\d+)\//
+/**
+ *
+ * Why not any free number? If we allowed it to be any integer number
+ * someone would put our backend servers at risk by doing something like:
+ *
+ *    const makeURL = () => `${BASE}/assets/mw-${Math.floor(Math.random()*1000)}/foo.png`
+ *    await Promise.all([...Array(10000).keys()].map(makeURL))
+ *
+ * Which would be lots of distinctly different and valid URLs that the
+ * CDN can never really "protect us" on because they're too often distinct.
+ *
+ * At the moment, the only business need is for 1,000 pixels, so the array
+ * only has one. But can change in the future and make this sentence moot.
+ */
+const VALID_MAX_WIDTHS = [1000]
+
 export default async function dynamicAssets(req, res, next) {
   if (!req.url.startsWith('/assets/')) return next()
 
@@ -44,27 +73,38 @@ export default async function dynamicAssets(req, res, next) {
     return res.redirect(302, req.path)
   }
 
+  // From PNG to WEBP, if the PNG exists
   if (req.path.endsWith('.webp')) {
-    // From PNG (if it exists) to WEBP
+    const { url, maxWidth, error } = deconstructImageURL(req.path)
+    if (error) {
+      return res.status(400).type('text/plain').send(error.toString())
+    }
     try {
-      const originalBuffer = await fs.readFile(req.path.slice(1).replace(/\.webp$/, '.png'))
-      const buffer = await sharp(originalBuffer)
-        // Note that by default, sharp will use a lossy compression.
-        // (i.e. `{lossless: false}` in the options)
-        // The difference is that a lossless image is slightly crisper
-        // but becomes on average 1.8x larger.
-        // Given how we serve images, no human would be able to tell the
-        // difference simply by looking at the image as it appears as an
-        // image tag in the web page.
-        // Also given that rendering-for-viewing is the "end of the line"
-        // for the image meaning it just ends up being viewed and not
-        // resaved as a source file. If we had intention to overwrite all
-        // original PNG source files to WEBP, we should consier lossless
-        // to preserve as much quality as possible at the source level.
-        // The default quality is 80% which, combined with `lossless:false`
-        // makes our images 2.8x smaller than the average PNG.
-        .webp()
-        .toBuffer()
+      const originalBuffer = await fs.readFile(url.slice(1).replace(/\.webp$/, '.png'))
+      const image = sharp(originalBuffer)
+
+      if (maxWidth) {
+        const { width } = await image.metadata()
+        if (width > maxWidth) {
+          image.resize({ width: maxWidth })
+        }
+      }
+
+      // Note that by default, sharp will use a lossy compression.
+      // (i.e. `{lossless: false}` in the options)
+      // The difference is that a lossless image is slightly crisper
+      // but becomes on average 1.8x larger.
+      // Given how we serve images, no human would be able to tell the
+      // difference simply by looking at the image as it appears as an
+      // image tag in the web page.
+      // Also given that rendering-for-viewing is the "end of the line"
+      // for the image meaning it just ends up being viewed and not
+      // resaved as a source file. If we had intention to overwrite all
+      // original PNG source files to WEBP, we should consier lossless
+      // to preserve as much quality as possible at the source level.
+      // The default quality is 80% which, combined with `lossless:false`
+      // makes our images 2.8x smaller than the average PNG.
+      const buffer = await image.webp().toBuffer()
       assetCacheControl(res)
       return res.type('image/webp').send(buffer)
     } catch (error) {
@@ -74,11 +114,23 @@ export default async function dynamicAssets(req, res, next) {
     }
   }
 
+  // From PNG to AVIF, if the PNG exists
   if (req.path.endsWith('.avif')) {
-    // From PNG (if it exists) to AVIF
+    const { url, maxWidth, error } = deconstructImageURL(req.path)
+    if (error) {
+      return res.status(400).type('text/plain').send(error.toString())
+    }
     try {
-      const originalBuffer = await fs.readFile(req.path.slice(1).replace(/\.avif$/, '.png'))
-      const buffer = await sharp(originalBuffer)
+      const originalBuffer = await fs.readFile(url.slice(1).replace(/\.avif$/, '.png'))
+      const image = sharp(originalBuffer)
+
+      if (maxWidth) {
+        const { width } = await image.metadata()
+        if (width > maxWidth) {
+          image.resize({ width: maxWidth })
+        }
+      }
+      const buffer = await image
         .avif({
           // The default is 4 (max is 9). Because this is a dynamic thing
           // and AVIF encoding is slow for large images, go for a smaller
@@ -114,3 +166,19 @@ export default async function dynamicAssets(req, res, next) {
   // broken link, like it might be to a regular HTML page.
   res.status(404).type('text/plain').send('Asset not found')
 }
+
+function deconstructImageURL(url) {
+  let error
+  let maxWidth
+  const match = url.match(maxWidthPathPartRegex)
+  if (match) {
+    const [whole, number] = match
+    maxWidth = parseInt(number)
+    if (isNaN(maxWidth) || maxWidth <= 0 || !VALID_MAX_WIDTHS.includes(maxWidth)) {
+      error = new Error(`width number (${maxWidth}) is not a valid number`)
+    } else {
+      url = url.replace(whole, '/')
+    }
+  }
+  return { url, maxWidth, error }
+}

tests/fixtures/content/get-started/foo/index.md

@@ -12,4 +12,5 @@ children:
   - /autotitling
   - /typo-autotitling
   - /cross-version-linking
+  - /single-image
 ---

tests/fixtures/content/get-started/foo/single-image.md

@@ -0,0 +1,14 @@
+---
+title: Single image
+intro: A simple page that has 1 asset image
+versions:
+  fpt: '*'
+  ghes: '*'
+  ghae: '*'
+  ghec: '*'
+type: how_to
+---
+
+## An image
+
+![This is the alt text](/assets/images/_fixtures/screenshot.png)

tests/helpers/e2etest.js

@@ -10,6 +10,7 @@ export async function get(
     followRedirects: false,
     followAllRedirects: false,
     headers: {},
+    responseType: undefined,
   }
 ) {
   const method = opts.method || 'get'
@@ -23,6 +24,7 @@ export async function get(
       retry: { limit: 0 },
       throwHttpErrors: false,
       followRedirect: opts.followAllRedirects || opts.followRedirects,
+      responseType: opts.responseType,
     },
     isUndefined
   )