From 0b12e2e8595c67fdb8ffdd36c189916b59df5ce7 Mon Sep 17 00:00:00 2001 From: Jan Potoms <2109932+Janpot@users.noreply.github.com> Date: Thu, 13 Feb 2020 05:39:51 +0100 Subject: [PATCH] Improve error for invalid page configurations (#10441) * Remove any type and fix edge cases Removed the "as any" and use the @babel/types typeguards instead. This revealed some edge cases that would just error. * Remove ts-ignore Co-authored-by: Joe Haddad --- .../build/babel/plugins/next-page-config.ts | 54 ++++++++++---- test/integration/page-config/pages/index.js | 2 - .../pages/invalid/invalid-property.js | 3 + .../pages/invalid/invalid-value.js | 5 ++ .../page-config/pages/invalid/no-init.js | 3 + .../pages/invalid/spread-config.js | 5 ++ .../pages/invalid/string-config.js | 3 + .../page-config/test/index.test.js | 71 +++++++++++++++++-- 8 files changed, 123 insertions(+), 23 deletions(-) create mode 100644 test/integration/page-config/pages/invalid/invalid-property.js create mode 100644 test/integration/page-config/pages/invalid/invalid-value.js create mode 100644 test/integration/page-config/pages/invalid/no-init.js create mode 100644 test/integration/page-config/pages/invalid/spread-config.js create mode 100644 test/integration/page-config/pages/invalid/string-config.js diff --git a/packages/next/build/babel/plugins/next-page-config.ts b/packages/next/build/babel/plugins/next-page-config.ts index 9fb733db40a16..c968e15c73a94 100644 --- a/packages/next/build/babel/plugins/next-page-config.ts +++ b/packages/next/build/babel/plugins/next-page-config.ts @@ -2,7 +2,6 @@ import { NodePath, PluginObj } from '@babel/core' import * as BabelTypes from '@babel/types' import { PageConfig } from 'next/types' -const configKeys = new Set(['amp']) const STRING_LITERAL_DROP_BUNDLE = '__NEXT_DROP_CLIENT_FILE__' // replace program path with just a variable with the drop identifier @@ -26,6 +25,12 @@ function replaceBundle(path: any, t: typeof BabelTypes) { ) } +function errorMessage(state: any, details: string) { + const pageName = + (state.filename || '').split(state.cwd || '').pop() || 'unknown' + return `Invalid page config export found. ${details} in file ${pageName}. See: https://err.sh/zeit/next.js/invalid-page-config` +} + interface ConfigState { bundleDropped?: boolean } @@ -50,32 +55,51 @@ export default function nextPageConfig({ return } - const { declarations } = path.node.declaration as any - const config: PageConfig = {} - - if (!declarations) { + if (!BabelTypes.isVariableDeclaration(path.node.declaration)) { return } + + const { declarations } = path.node.declaration + const config: PageConfig = {} + for (const declaration of declarations) { - if (declaration.id.name !== 'config') { + if ( + !BabelTypes.isIdentifier(declaration.id, { name: 'config' }) + ) { continue } - if (declaration.init.type !== 'ObjectExpression') { - const pageName = - (state.filename || '').split(state.cwd || '').pop() || - 'unknown' - + if (!BabelTypes.isObjectExpression(declaration.init)) { + const got = declaration.init + ? declaration.init.type + : 'undefined' throw new Error( - `Invalid page config export found. Expected object but got ${declaration.init.type} in file ${pageName}. See: https://err.sh/zeit/next.js/invalid-page-config` + errorMessage(state, `Expected object but got ${got}`) ) } for (const prop of declaration.init.properties) { + if (BabelTypes.isSpreadElement(prop)) { + throw new Error( + errorMessage(state, `Property spread is not allowed`) + ) + } const { name } = prop.key - if (configKeys.has(name)) { - // @ts-ignore - config[name] = prop.value.value + if (BabelTypes.isIdentifier(prop.key, { name: 'amp' })) { + if (!BabelTypes.isObjectProperty(prop)) { + throw new Error( + errorMessage(state, `Invalid property "${name}"`) + ) + } + if ( + !BabelTypes.isBooleanLiteral(prop.value) && + !BabelTypes.isStringLiteral(prop.value) + ) { + throw new Error( + errorMessage(state, `Invalid value for "${name}"`) + ) + } + config.amp = prop.value.value as PageConfig['amp'] } } } diff --git a/test/integration/page-config/pages/index.js b/test/integration/page-config/pages/index.js index 8f84e202bb551..7daea25f7d900 100644 --- a/test/integration/page-config/pages/index.js +++ b/test/integration/page-config/pages/index.js @@ -1,8 +1,6 @@ import { config as hello } from '../something' import { config as world } from '../config' -// export const config = 'hello world' - export default () => (

{hello} {world} diff --git a/test/integration/page-config/pages/invalid/invalid-property.js b/test/integration/page-config/pages/invalid/invalid-property.js new file mode 100644 index 0000000000000..fae2214867d3f --- /dev/null +++ b/test/integration/page-config/pages/invalid/invalid-property.js @@ -0,0 +1,3 @@ +// export const config = { amp() {} } + +export default () =>

hello world

diff --git a/test/integration/page-config/pages/invalid/invalid-value.js b/test/integration/page-config/pages/invalid/invalid-value.js new file mode 100644 index 0000000000000..10fcd812d31fb --- /dev/null +++ b/test/integration/page-config/pages/invalid/invalid-value.js @@ -0,0 +1,5 @@ +const amp = 'hybrid' + +// export const config = { amp } + +export default () =>

hello ${amp}

diff --git a/test/integration/page-config/pages/invalid/no-init.js b/test/integration/page-config/pages/invalid/no-init.js new file mode 100644 index 0000000000000..fc1b6e41d4f00 --- /dev/null +++ b/test/integration/page-config/pages/invalid/no-init.js @@ -0,0 +1,3 @@ +// export let 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 new file mode 100644 index 0000000000000..93e2e73dda7cf --- /dev/null +++ b/test/integration/page-config/pages/invalid/spread-config.js @@ -0,0 +1,5 @@ +const props = { amp: true } + +// export const config = { ...props } + +export default () =>

{props}

diff --git a/test/integration/page-config/pages/invalid/string-config.js b/test/integration/page-config/pages/invalid/string-config.js new file mode 100644 index 0000000000000..c3d4d570d0ee4 --- /dev/null +++ b/test/integration/page-config/pages/invalid/string-config.js @@ -0,0 +1,3 @@ +// export const config = 'hello world' + +export default () =>

hello world

diff --git a/test/integration/page-config/test/index.test.js b/test/integration/page-config/test/index.test.js index 7adb5316169ba..f4aca874c052e 100644 --- a/test/integration/page-config/test/index.test.js +++ b/test/integration/page-config/test/index.test.js @@ -5,20 +5,79 @@ import { join } from 'path' import { nextBuild } from 'next-test-utils' const appDir = join(__dirname, '..') -const indexPage = join(appDir, 'pages/index.js') jasmine.DEFAULT_TIMEOUT_INTERVAL = 1000 * 60 * 2 +async function uncommentExport(page) { + const pagePath = join(appDir, 'pages', page) + const origContent = await fs.readFile(pagePath, 'utf8') + const newContent = origContent.replace('// export', 'export') + await fs.writeFile(pagePath, newContent, 'utf8') + return async () => { + await fs.writeFile(pagePath, origContent, 'utf8') + } +} + describe('Page Config', () => { it('builds without error when export const config is used outside page', async () => { const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) expect(stderr).not.toMatch(/Failed to compile\./) }) - it('shows valid error on invalid page config', async () => { - const origContent = await fs.readFile(indexPage, 'utf8') - const newContent = origContent.replace('// export', 'export') - await fs.writeFile(indexPage, newContent, 'utf8') + it('shows valid error when page config is a string', async () => { + const reset = await uncommentExport('invalid/string-config.js') + + try { + const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) + expect(stderr).toMatch( + /https:\/\/err\.sh\/zeit\/next\.js\/invalid-page-config/ + ) + } finally { + await reset() + } + }) + + it('shows valid error when page config has no init', async () => { + const reset = await uncommentExport('invalid/no-init.js') + + try { + const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) + expect(stderr).toMatch( + /https:\/\/err\.sh\/zeit\/next\.js\/invalid-page-config/ + ) + } finally { + await reset() + } + }) + + it('shows error when page config has spread properties', async () => { + const reset = await uncommentExport('invalid/spread-config.js') + + try { + const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) + expect(stderr).toMatch( + /https:\/\/err\.sh\/zeit\/next\.js\/invalid-page-config/ + ) + } finally { + await reset() + } + }) + + it('shows error when page config has invalid properties', async () => { + const reset = await uncommentExport('invalid/invalid-property.js') + + try { + const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) + expect(stderr).toMatch( + /https:\/\/err\.sh\/zeit\/next\.js\/invalid-page-config/ + ) + } finally { + await reset() + } + }) + + it('shows error when page config has invalid property value', async () => { + const reset = await uncommentExport('invalid/invalid-value.js') try { const { stderr } = await nextBuild(appDir, undefined, { stderr: true }) @@ -26,7 +85,7 @@ describe('Page Config', () => { /https:\/\/err\.sh\/zeit\/next\.js\/invalid-page-config/ ) } finally { - await fs.writeFile(indexPage, origContent, 'utf8') + await reset() } }) })