Add AMP validation on export (vercel#6794)

* Add err.sh link and pool validation results
to wait to show error until export is finished

* Fix wording in amp-export-validation err.sh

* Update validation error message

Co-Authored-By: ijjk <[email protected]>

* Update ways to fix text

Co-Authored-By: ijjk <[email protected]>

* Update why the error occurred wording

Co-Authored-By: ijjk <[email protected]>

* Update wording some more

Co-Authored-By: ijjk <[email protected]>

errors/amp-export-validation.md

@@ -0,0 +1,15 @@
+# AMP Export Validation
+
+#### Why This Error Occurred
+
+During export we validate AMP pages using [amphtml-validator](https://www.npmjs.com/package/amphtml-validator). Errors received from the validator will fail the export.
+
+Validation errors will cause pages to not be [indexed by AMP Caches](https://www.ampproject.org/docs/fundamentals/how_cached).
+
+#### Possible Ways to Fix It
+
+Look at the error messages and follow their appropriate links to learn more about the error and how to correct the problem.
+
+### Useful Links
+
+- [AMP HTML Specification](https://www.ampproject.org/docs/fundamentals/spec)

packages/next/build/output/index.ts

@@ -58,7 +58,7 @@ function getWebpackStatusPhase(status: WebpackStatus): WebpackStatusPhase {
   return WebpackStatusPhase.COMPILED
 }
 
-function formatAmpMessages(amp: AmpPageStatus) {
+export function formatAmpMessages(amp: AmpPageStatus) {
   let output = chalk.bold('Amp Validation') + '\n\n'
   let messages: string[][] = []
 

packages/next/export/index.js

@@ -11,6 +11,7 @@ import { PHASE_EXPORT, SERVER_DIRECTORY, PAGES_MANIFEST, CONFIG_FILE, BUILD_ID_F
 import createProgress from 'tty-aware-progress'
 import { promisify } from 'util'
 import { recursiveDelete } from '../lib/recursive-delete'
+import { formatAmpMessages } from '../build/output/index'
 
 const mkdirp = promisify(mkdirpModule)
 
@@ -143,6 +144,9 @@ export default async function (dir, options, configuration) {
     return result
   }, [])
 
+  const ampValidations = {}
+  let hadValidationError = false
+
   await Promise.all(
     chunks.map(
       chunk =>
@@ -167,12 +171,22 @@ export default async function (dir, options, configuration) {
               reject(payload)
             } else if (type === 'done') {
               resolve()
+            } else if (type === 'amp-validation') {
+              ampValidations[payload.page] = payload.result
+              hadValidationError = hadValidationError || payload.result.errors.length
             }
           })
         })
     )
   )
 
+  if (Object.keys(ampValidations).length) {
+    console.log(formatAmpMessages(ampValidations))
+  }
+  if (hadValidationError) {
+    throw new Error(`AMP Validation caused the export to fail. https://err.sh/zeit/next.js/amp-export-validation`)
+  }
+
   // Add an empty line to the console for the better readability.
   log('')
 }

packages/next/export/worker.js

@@ -5,6 +5,7 @@ import { cleanAmpPath } from 'next-server/dist/server/utils'
 import { renderToHTML } from 'next-server/dist/server/render'
 import { writeFile } from 'fs'
 import Sema from 'async-sema'
+import AmpHtmlValidator from 'amphtml-validator'
 import { loadComponents } from 'next-server/dist/server/load-components'
 
 const envConfig = require('next-server/config')
@@ -67,6 +68,27 @@ process.on(
         await mkdirp(baseDir)
         const components = await loadComponents(distDir, buildId, page)
         const html = await renderToHTML(req, res, page, query, { ...components, ...renderOpts, ...ampOpts })
+
+        if (ampOpts.amphtml) {
+          const validator = await AmpHtmlValidator.getInstance()
+          const result = validator.validateString(html)
+          const errors = result.errors.filter(e => e.severity === 'ERROR')
+          const warnings = result.errors.filter(e => e.severity !== 'ERROR')
+
+          if (warnings.length || errors.length) {
+            process.send({
+              type: 'amp-validation',
+              payload: {
+                page,
+                result: {
+                  errors,
+                  warnings
+                }
+              }
+            })
+          }
+        }
+
         await new Promise((resolve, reject) =>
           writeFile(
             htmlFilepath,

test/integration/amp-export-validation/next.config.js

@@ -0,0 +1,6 @@
+module.exports = {
+  // exportPathMap
+  experimental: {
+    amp: true
+  }
+}

test/integration/amp-export-validation/pages/cat.amp.js

@@ -0,0 +1,6 @@
+export default () => (
+  <div>
+    {/* I show a warning since the amp-video script isn't added */}
+    <amp-video src='/cats.mp4' height={400} width={800} />
+  </div>
+)

test/integration/amp-export-validation/pages/dog-cat.amp.js

@@ -0,0 +1,8 @@
+export default () => (
+  <div>
+    {/* I throw an error since <amp-img/> should be used instead */}
+    <img src='/dog.gif' height={400} width={800} />
+    {/* I show a warning since the amp-video script isn't added */}
+    <amp-video src='/cats.mp4' height={400} width={800} />
+  </div>
+)

test/integration/amp-export-validation/pages/dog.amp.js

@@ -0,0 +1,6 @@
+export default () => (
+  <div>
+    {/* I throw an error since <amp-img/> should be used instead */}
+    <img src='/dog.gif' height={400} width={800} />
+  </div>
+)

test/integration/amp-export-validation/test/index.test.js

@@ -0,0 +1,93 @@
+/* eslint-env jest */
+/* global jasmine */
+import fs from 'fs'
+import { join } from 'path'
+import { promisify } from 'util'
+import {
+  File,
+  nextBuild,
+  runNextCommand
+} from 'next-test-utils'
+
+jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 5
+const access = promisify(fs.access)
+const appDir = join(__dirname, '../')
+const outDir = join(appDir, 'out')
+const nextConfig = new File(join(appDir, 'next.config.js'))
+
+describe('AMP Validation on Export', () => {
+  beforeAll(() => nextBuild(appDir))
+
+  it('shows AMP warning without throwing error', async () => {
+    nextConfig.replace('// exportPathMap',
+      `exportPathMap: function(defaultMap) {
+      return {
+        '/cat': defaultMap['/cat.amp'],
+      }
+    },`)
+
+    let stdout = ''
+    try {
+      await runNextCommand(['export', appDir], {
+        instance: child => {
+          child.stdout.on('data', chunk => {
+            stdout += chunk.toString()
+          })
+        }
+      })
+      expect(stdout).toMatch(/warn.*The tag 'amp-video extension \.js script' is missing/)
+      await expect(access(join(outDir, 'cat/index.html'))).resolves.toBe(undefined)
+    } finally {
+      nextConfig.restore()
+    }
+  })
+
+  it('throws error on AMP error', async () => {
+    nextConfig.replace('// exportPathMap',
+      `exportPathMap: function(defaultMap) {
+      return {
+        '/dog': defaultMap['/dog.amp'],
+      }
+    },`)
+
+    let stdout = ''
+    try {
+      await runNextCommand(['export', appDir], {
+        instance: child => {
+          child.stdout.on('data', chunk => {
+            stdout += chunk.toString()
+          })
+        }
+      })
+      expect(stdout).toMatch(/error.*The tag 'img' may only appear as a descendant of tag 'noscript'. Did you mean 'amp-img'\?/)
+      await expect(access(join(outDir, 'dog/index.html'))).resolves.toBe(undefined)
+    } finally {
+      nextConfig.restore()
+    }
+  })
+
+  it('shows warning and error when throwing error', async () => {
+    nextConfig.replace('// exportPathMap',
+      `exportPathMap: function(defaultMap) {
+      return {
+        '/dog-cat': defaultMap['/dog-cat.amp'],
+      }
+    },`)
+
+    let stdout = ''
+    try {
+      await runNextCommand(['export', appDir], {
+        instance: child => {
+          child.stdout.on('data', chunk => {
+            stdout += chunk.toString()
+          })
+        }
+      })
+      expect(stdout).toMatch(/warn.*The tag 'amp-video extension \.js script' is missing/)
+      expect(stdout).toMatch(/error.*The tag 'img' may only appear as a descendant of tag 'noscript'. Did you mean 'amp-img'\?/)
+      await expect(access(join(outDir, 'dog-cat/index.html'))).resolves.toBe(undefined)
+    } finally {
+      nextConfig.restore()
+    }
+  })
+})