From 244b0e700eed61924b1790ebf4b83e611d9e6fd7 Mon Sep 17 00:00:00 2001 From: JJ Kasper <22380829+ijjk@users.noreply.github.com> Date: Tue, 26 Mar 2019 16:21:27 -0500 Subject: [PATCH] Add AMP validation on export (#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 <22380829+ijjk@users.noreply.github.com> * Update ways to fix text Co-Authored-By: ijjk <22380829+ijjk@users.noreply.github.com> * Update why the error occurred wording Co-Authored-By: ijjk <22380829+ijjk@users.noreply.github.com> * Update wording some more Co-Authored-By: ijjk <22380829+ijjk@users.noreply.github.com> --- errors/amp-export-validation.md | 15 +++ packages/next/build/output/index.ts | 2 +- packages/next/export/index.js | 14 +++ packages/next/export/worker.js | 22 +++++ .../amp-export-validation/next.config.js | 6 ++ .../amp-export-validation/pages/cat.amp.js | 6 ++ .../pages/dog-cat.amp.js | 8 ++ .../amp-export-validation/pages/dog.amp.js | 6 ++ .../amp-export-validation/test/index.test.js | 93 +++++++++++++++++++ 9 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 errors/amp-export-validation.md create mode 100644 test/integration/amp-export-validation/next.config.js create mode 100644 test/integration/amp-export-validation/pages/cat.amp.js create mode 100644 test/integration/amp-export-validation/pages/dog-cat.amp.js create mode 100644 test/integration/amp-export-validation/pages/dog.amp.js create mode 100644 test/integration/amp-export-validation/test/index.test.js diff --git a/errors/amp-export-validation.md b/errors/amp-export-validation.md new file mode 100644 index 0000000000000..bb87e7b42f30d --- /dev/null +++ b/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) diff --git a/packages/next/build/output/index.ts b/packages/next/build/output/index.ts index a3bcce67279a1..5d4680a635691 100644 --- a/packages/next/build/output/index.ts +++ b/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[][] = [] diff --git a/packages/next/export/index.js b/packages/next/export/index.js index 392edcd1eb7e8..1b5703b6a2474 100644 --- a/packages/next/export/index.js +++ b/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('') } diff --git a/packages/next/export/worker.js b/packages/next/export/worker.js index 230b097bdc257..5118819015548 100644 --- a/packages/next/export/worker.js +++ b/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, diff --git a/test/integration/amp-export-validation/next.config.js b/test/integration/amp-export-validation/next.config.js new file mode 100644 index 0000000000000..a0462b7feabcf --- /dev/null +++ b/test/integration/amp-export-validation/next.config.js @@ -0,0 +1,6 @@ +module.exports = { + // exportPathMap + experimental: { + amp: true + } +} diff --git a/test/integration/amp-export-validation/pages/cat.amp.js b/test/integration/amp-export-validation/pages/cat.amp.js new file mode 100644 index 0000000000000..d61a0d200131d --- /dev/null +++ b/test/integration/amp-export-validation/pages/cat.amp.js @@ -0,0 +1,6 @@ +export default () => ( +
+ {/* I show a warning since the amp-video script isn't added */} + +
+) diff --git a/test/integration/amp-export-validation/pages/dog-cat.amp.js b/test/integration/amp-export-validation/pages/dog-cat.amp.js new file mode 100644 index 0000000000000..9f7aa368927dc --- /dev/null +++ b/test/integration/amp-export-validation/pages/dog-cat.amp.js @@ -0,0 +1,8 @@ +export default () => ( +
+ {/* I throw an error since should be used instead */} + + {/* I show a warning since the amp-video script isn't added */} + +
+) diff --git a/test/integration/amp-export-validation/pages/dog.amp.js b/test/integration/amp-export-validation/pages/dog.amp.js new file mode 100644 index 0000000000000..0a2dab143a218 --- /dev/null +++ b/test/integration/amp-export-validation/pages/dog.amp.js @@ -0,0 +1,6 @@ +export default () => ( +
+ {/* I throw an error since should be used instead */} + +
+) diff --git a/test/integration/amp-export-validation/test/index.test.js b/test/integration/amp-export-validation/test/index.test.js new file mode 100644 index 0000000000000..6b5b1b57efda9 --- /dev/null +++ b/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() + } + }) +})