From aa4b87e357f1881834ba93b2f33ba2a91e49ecce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kevin=20M=C3=A5rtensson?= Date: Mon, 17 Aug 2020 19:24:18 +0200 Subject: [PATCH] Handle cases where `config` is exported after its declaration (#16211) AMP will still not work correctly when switching between non-AMP and AMP pages in development mode because of https://github.com/vercel/next.js/blob/canary/packages/next/build/babel/plugins/next-page-config.ts#L116-L120. Fixes #15704. --- errors/invalid-page-config.md | 15 +++++- .../build/babel/plugins/next-page-config.ts | 48 ++++++++++++++++--- test/integration/amphtml/pages/nav.js | 5 ++ .../amphtml/pages/var-before-export.js | 12 +++++ test/integration/amphtml/test/index.test.js | 18 +++++++ .../page-config/pages/invalid/export-from.js | 3 ++ .../pages/invalid/import-export.js | 6 +++ .../pages/invalid/spread-config.js | 2 +- .../page-config/test/index.test.js | 26 ++++++++++ 9 files changed, 126 insertions(+), 9 deletions(-) create mode 100644 test/integration/amphtml/pages/var-before-export.js create mode 100644 test/integration/page-config/pages/invalid/export-from.js create mode 100644 test/integration/page-config/pages/invalid/import-export.js diff --git a/errors/invalid-page-config.md b/errors/invalid-page-config.md index 327e40d821..6984a9bb1b 100644 --- a/errors/invalid-page-config.md +++ b/errors/invalid-page-config.md @@ -6,7 +6,7 @@ In one of your pages you did `export const config` with an invalid value. #### Possible Ways to Fix It -The page's config must be an object initialized directly when being exported. +The page's config must be an object initialized directly when being exported and not modified dynamically. This is not allowed @@ -14,6 +14,19 @@ This is not allowed export const config = 'hello world' ``` +This is not allowed + +```js +const config = {} +config.amp = true +``` + +This is not allowed + +```js +export { config } from '../config' +``` + This is allowed ```js diff --git a/packages/next/build/babel/plugins/next-page-config.ts b/packages/next/build/babel/plugins/next-page-config.ts index adf724d854..1e836f23dd 100644 --- a/packages/next/build/babel/plugins/next-page-config.ts +++ b/packages/next/build/babel/plugins/next-page-config.ts @@ -2,6 +2,8 @@ import { NodePath, PluginObj, types as BabelTypes } from '@babel/core' import { PageConfig } from 'next/types' import { STRING_LITERAL_DROP_BUNDLE } from '../../../next-server/lib/constants' +const CONFIG_KEY = 'config' + // replace program path with just a variable with the drop identifier function replaceBundle(path: any, t: typeof BabelTypes): void { path.parentPath.replaceWith( @@ -41,27 +43,59 @@ export default function nextPageConfig({ enter(path, state: ConfigState) { path.traverse( { + ExportDeclaration(exportPath, exportState) { + if ( + BabelTypes.isExportNamedDeclaration(exportPath) && + (exportPath.node as BabelTypes.ExportNamedDeclaration).specifiers?.some( + (specifier) => { + return specifier.exported.name === CONFIG_KEY + } + ) && + BabelTypes.isStringLiteral( + (exportPath.node as BabelTypes.ExportNamedDeclaration) + .source + ) + ) { + throw new Error( + errorMessage( + exportState, + 'Expected object but got export from' + ) + ) + } + }, ExportNamedDeclaration( exportPath: NodePath, exportState: any ) { - if (exportState.bundleDropped || !exportPath.node.declaration) { - return - } - if ( - !BabelTypes.isVariableDeclaration(exportPath.node.declaration) + exportState.bundleDropped || + (!exportPath.node.declaration && + exportPath.node.specifiers.length === 0) ) { return } - const { declarations } = exportPath.node.declaration const config: PageConfig = {} + const declarations = [ + ...(exportPath.node.declaration?.declarations || []), + exportPath.scope.getBinding(CONFIG_KEY)?.path.node, + ].filter(Boolean) for (const declaration of declarations) { if ( - !BabelTypes.isIdentifier(declaration.id, { name: 'config' }) + !BabelTypes.isIdentifier(declaration.id, { + name: CONFIG_KEY, + }) ) { + if (BabelTypes.isImportSpecifier(declaration)) { + throw new Error( + errorMessage( + exportState, + `Expected object but got import` + ) + ) + } continue } diff --git a/test/integration/amphtml/pages/nav.js b/test/integration/amphtml/pages/nav.js index f3d38a2491..2cc684c0dc 100644 --- a/test/integration/amphtml/pages/nav.js +++ b/test/integration/amphtml/pages/nav.js @@ -12,6 +12,11 @@ export default function Nav() { AMP First Page +
  • + + Another AMP First Page + +
    • ) } diff --git a/test/integration/amphtml/pages/var-before-export.js b/test/integration/amphtml/pages/var-before-export.js new file mode 100644 index 0000000000..31fdc2a36b --- /dev/null +++ b/test/integration/amphtml/pages/var-before-export.js @@ -0,0 +1,12 @@ +const config = { amp: true } + +const Page = () => { + return ( +
    +

    Only AMP for me...

    +
    + ) +} + +export default Page +export { config } diff --git a/test/integration/amphtml/test/index.test.js b/test/integration/amphtml/test/index.test.js index a9e730f948..37779e0d01 100644 --- a/test/integration/amphtml/test/index.test.js +++ b/test/integration/amphtml/test/index.test.js @@ -74,6 +74,15 @@ describe('AMP Usage', () => { expect(result).toBe(null) }) + it('should not output client pages for AMP only with config exported after declaration', async () => { + const browser = await webdriver(appPort, '/nav') + await browser.elementByCss('#var-before-export-link').click() + + const result = await browser.eval('window.NAV_PAGE_LOADED') + + expect(result).toBe(null) + }) + it('should add link preload for amp script', async () => { const html = await renderViaHTTP(appPort, '/?amp=1') await validateAMP(html) @@ -267,6 +276,15 @@ describe('AMP Usage', () => { expect(result).toBe(null) }) + + it('should not output client pages for AMP only with config exported after declaration', async () => { + const browser = await webdriver(appPort, '/nav') + await browser.elementByCss('#var-before-export-link').click() + + const result = await browser.eval('window.NAV_PAGE_LOADED') + + expect(result).toBe(null) + }) }) describe('AMP dev no-warn', () => { diff --git a/test/integration/page-config/pages/invalid/export-from.js b/test/integration/page-config/pages/invalid/export-from.js new file mode 100644 index 0000000000..ddb258d6cd --- /dev/null +++ b/test/integration/page-config/pages/invalid/export-from.js @@ -0,0 +1,3 @@ +// export { config } from '../../config' + +export default () =>

    hello world

    diff --git a/test/integration/page-config/pages/invalid/import-export.js b/test/integration/page-config/pages/invalid/import-export.js new file mode 100644 index 0000000000..919272bb86 --- /dev/null +++ b/test/integration/page-config/pages/invalid/import-export.js @@ -0,0 +1,6 @@ +// eslint-disable-next-line no-unused-vars +import { config } from '../../config' + +// export { config } + +export default () =>

    hello world

    diff --git a/test/integration/page-config/pages/invalid/spread-config.js b/test/integration/page-config/pages/invalid/spread-config.js index 93e2e73dda..a85262c17e 100644 --- a/test/integration/page-config/pages/invalid/spread-config.js +++ b/test/integration/page-config/pages/invalid/spread-config.js @@ -2,4 +2,4 @@ const props = { amp: true } // export const config = { ...props } -export default () =>

    {props}

    +export default () =>

    {JSON.stringify(props)}

    diff --git a/test/integration/page-config/test/index.test.js b/test/integration/page-config/test/index.test.js index 01933be567..a1d8b649be 100644 --- a/test/integration/page-config/test/index.test.js +++ b/test/integration/page-config/test/index.test.js @@ -88,4 +88,30 @@ describe('Page Config', () => { await reset() } }) + + it('shows error when page config is export from', async () => { + const reset = await uncommentExport('invalid/export-from.js') + + try { + const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) + expect(stderr).toMatch( + /https:\/\/err\.sh\/vercel\/next\.js\/invalid-page-config/ + ) + } finally { + await reset() + } + }) + + it('shows error when page config is imported and exported', async () => { + const reset = await uncommentExport('invalid/import-export.js') + + try { + const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) + expect(stderr).toMatch( + /https:\/\/err\.sh\/vercel\/next\.js\/invalid-page-config/ + ) + } finally { + await reset() + } + }) })