Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#38743): config.runtime support template literal #38750

Merged
merged 13 commits into from Jul 21, 2022
38 changes: 36 additions & 2 deletions errors/invalid-page-config.md
@@ -1,12 +1,13 @@
# Invalid Page Config
# Invalid Page / API Route Config

#### Why This Error Occurred

In one of your pages you did `export const config` with an invalid value.
In one of your pages or API Routes 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 and not modified dynamically.
The config object must only contains static constant literals without expressions.

This is not allowed

Expand All @@ -23,17 +24,50 @@ config.amp = true

This is not allowed

```js
export const config = {
amp: 1 + 1 > 2,
}
```

This is not allowed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: it is hard to spot what is allowed and what is not.

What about using a table, or bold, or any artifact helping readers to immediately find what is allowed and what is not?

Not allowedAllowed
export const config = {
  amp: 1 + 1 > 2,
}
export const config = {
  amp: false,
}
export const config = {
  runtime: `n${'od'}ejs`
}

export const config = {
  runtime: (() => 'nodejs')()
}
export const config = {
  runtime: `nodejs`,
}

export const config = {
  runtime: 'nodejs',
}

Copy link
Contributor Author

@SukkaW SukkaW Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feugy Agree.

And an alternative I have thought of is to replicate the ESLint documentation style:
https://eslint.org/docs/latest/rules/valid-typeof

Copy link
Contributor Author

@SukkaW SukkaW Jul 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 1858c1a (#38750)

```js
export { config } from '../config'
```

This is not allowed

```js
export const config = {
runtime: `n${'od'}ejs`,
}
```

This is allowed

```js
export const config = { amp: true }
```

This is allowed

```js
export const config = {
runtime: 'nodejs',
}
```

This is allowed

```js
export const config = {
runtime: `nodejs`,
}
```

### Useful Links

- [Enabling AMP Support](https://nextjs.org/docs/advanced-features/amp-support/introduction)
- [API Middlewares](https://nextjs.org/docs/api-routes/api-middlewares)
- [Switchable Runtime](https://nextjs.org/docs/advanced-features/react-18/switchable-runtime)
28 changes: 26 additions & 2 deletions packages/next/build/analysis/extract-const-value.ts
Expand Up @@ -11,6 +11,7 @@ import type {
ObjectExpression,
RegExpLiteral,
StringLiteral,
TemplateLiteral,
VariableDeclaration,
} from '@swc/core'

Expand Down Expand Up @@ -124,8 +125,12 @@ function isRegExpLiteral(node: Node): node is RegExpLiteral {
return node.type === 'RegExpLiteral'
}

class UnsupportedValueError extends Error {}
class NoSuchDeclarationError extends Error {}
function isTemplateLiteral(node: Node): node is TemplateLiteral {
return node.type === 'TemplateLiteral'
}

export class UnsupportedValueError extends Error {}
export class NoSuchDeclarationError extends Error {}

function extractValue(node: Node): any {
if (isNullLiteral(node)) {
Expand Down Expand Up @@ -191,6 +196,25 @@ function extractValue(node: Node): any {
}

return obj
} else if (isTemplateLiteral(node)) {
// e.g. `abc`
if (node.expressions.length !== 0) {
// TODO: should we add support for `${'e'}d${'g'}'e'`?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: It is fair to not support real interpolation.

The best option would be to load the file and check the exported value. However, that's not an option, so we can go with this tradeoff.

throw new UnsupportedValueError()
}

// When TemplateLiteral has 0 expressions, the length of quasis is always 1.
// Because when parsing TemplateLiteral, the parser yields the first quasi,
// then the first expression, then the next quasi, then the next expression, etc.,
// until the last quasi.
// Thus if there is no expression, the parser ends at the frst and also last quasis
//
// A "cooked" interpretation where backslashes have special meaning, while a
// "raw" interpretation where backslashes do not have special meaning
// https://exploringjs.com/impatient-js/ch_template-literals.html#template-strings-cooked-vs-raw
const [{ cooked, raw }] = node.quasis

return cooked ?? raw
} else {
throw new UnsupportedValueError()
}
Expand Down
35 changes: 32 additions & 3 deletions packages/next/build/analysis/get-page-static-info.ts
@@ -1,6 +1,9 @@
import { isServerRuntime, ServerRuntime } from '../../server/config-shared'
import type { NextConfig } from '../../server/config-shared'
import { tryToExtractExportedConstValue } from './extract-const-value'
import {
extractExportedConstValue,
UnsupportedValueError,
} from './extract-const-value'
import { escapeStringRegexp } from '../../shared/lib/escape-regexp'
import { parseModule } from './parse-module'
import { promises as fs } from 'fs'
Expand Down Expand Up @@ -32,13 +35,23 @@ export async function getPageStaticInfo(params: {
isDev?: boolean
page?: string
}): Promise<PageStaticInfo> {
const { isDev, pageFilePath, nextConfig } = params
const { isDev, pageFilePath, nextConfig, page } = params

const fileContent = (await tryToReadFile(pageFilePath, !isDev)) || ''
if (/runtime|getStaticProps|getServerSideProps|matcher/.test(fileContent)) {
const swcAST = await parseModule(pageFilePath, fileContent)
const { ssg, ssr } = checkExports(swcAST)
const config = tryToExtractExportedConstValue(swcAST, 'config') || {}

// default / failsafe value for config
let config: any = {}
try {
config = extractExportedConstValue(swcAST, 'config')
} catch (e) {
if (e instanceof UnsupportedValueError) {
warnAboutUnsupportedValue(pageFilePath, page)
}
// `export config` doesn't exist, or other unknown error throw by swc, silence them
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion(blocker): rather than discontinuing tryToExtractExportedConstValue, you could enhance it to print the warning.

Copy link
Contributor Author

@SukkaW SukkaW Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thought about adding the logging in tryToExtractExportedConstValue. But I'd like to include pageFilePath or route path in the warning message (the developer can then locate and identify the problematic file).

So I just use extractExportedConstValue directly instead, and I don't need to pass the context (route path, page file path, etc.) down to tryToExtractExportedConstValue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Let's get rid of tryToExtractExportedConstValue since it's not used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I have removed it in e34ba69 (#38750).


if (
typeof config.runtime !== 'string' &&
Expand Down Expand Up @@ -218,3 +231,19 @@ function warnAboutExperimentalEdgeApiFunctions() {
}

let warnedAboutExperimentalEdgeApiFunctions = false

const warnedUnsupportedValueMap = new Map<string, boolean>()
function warnAboutUnsupportedValue(
pageFilePath: string,
page: string | undefined
) {
if (warnedUnsupportedValueMap.has(pageFilePath)) {
return
}
Log.warn(
`You have exported a \`config\` field in ${
page ? `route "${page}"` : `"${pageFilePath}"`
} that Next.js can't recognize, so it will be ignored. See: https://nextjs.org/docs/messages/invalid-page-config`
)
warnedUnsupportedValueMap.set(pageFilePath, true)
}
42 changes: 36 additions & 6 deletions test/e2e/switchable-runtime/index.test.ts
Expand Up @@ -115,11 +115,15 @@ describe('Switchable runtime', () => {
)
})

it('should build /api/hello as an api route with edge runtime', async () => {
const response = await fetchViaHTTP(context.appPort, '/api/hello')
const text = await response.text()
it('should build /api/hello and /api/edge as an api route with edge runtime', async () => {
let response = await fetchViaHTTP(context.appPort, '/api/hello')
let text = await response.text()
expect(text).toMatch(/Hello from .+\/api\/hello/)

response = await fetchViaHTTP(context.appPort, '/api/edge')
text = await response.text()
expect(text).toMatch(/Returned by Edge API Route .+\/api\/edge/)

if (!(global as any).isNextDeploy) {
const manifest = await readJson(
join(context.appDir, '.next/server/middleware-manifest.json')
Expand All @@ -137,6 +141,17 @@ describe('Switchable runtime', () => {
regexp: '^/api/hello$',
wasm: [],
},
'/api/edge': {
env: [],
files: [
'server/edge-runtime-webpack.js',
'server/pages/api/edge.js',
],
name: 'pages/api/edge',
page: '/api/edge',
regexp: '^/api/edge$',
wasm: [],
},
},
})
}
Expand Down Expand Up @@ -235,11 +250,15 @@ describe('Switchable runtime', () => {
})
})

it('should build /api/hello as an api route with edge runtime', async () => {
const response = await fetchViaHTTP(context.appPort, '/api/hello')
const text = await response.text()
it('should build /api/hello and /api/edge as an api route with edge runtime', async () => {
let response = await fetchViaHTTP(context.appPort, '/api/hello')
let text = await response.text()
expect(text).toMatch(/Hello from .+\/api\/hello/)

response = await fetchViaHTTP(context.appPort, '/api/edge')
text = await response.text()
expect(text).toMatch(/Returned by Edge API Route .+\/api\/edge/)

if (!(global as any).isNextDeploy) {
const manifest = await readJson(
join(context.appDir, '.next/server/middleware-manifest.json')
Expand All @@ -257,6 +276,17 @@ describe('Switchable runtime', () => {
regexp: '^/api/hello$',
wasm: [],
},
'/api/edge': {
env: [],
files: [
'server/edge-runtime-webpack.js',
'server/pages/api/edge.js',
],
name: 'pages/api/edge',
page: '/api/edge',
regexp: '^/api/edge$',
wasm: [],
},
},
})
}
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/switchable-runtime/pages/api/edge.js
@@ -0,0 +1,7 @@
export default (req) => {
return new Response(`Returned by Edge API Route ${req.url}`)
}

export const config = {
runtime: `experimental-edge`,
}
Expand Up @@ -4,7 +4,7 @@ import path from 'path'
describe('Exported runtimes value validation', () => {
test('fails to build on malformed input', async () => {
const result = await nextBuild(
path.resolve(__dirname, './app'),
path.resolve(__dirname, './invalid-runtime/app'),
undefined,
{ stdout: true, stderr: true }
)
Expand All @@ -15,4 +15,19 @@ describe('Exported runtimes value validation', () => {
),
})
})

test('warns on unrecognized runtimes value', async () => {
const result = await nextBuild(
path.resolve(__dirname, './unsupported-syntax/app'),
undefined,
{ stdout: true, stderr: true }
)

expect(result).toMatchObject({
code: 0,
stderr: expect.stringContaining(
`You have exported a \`config\` field in route "/" that Next.js can't recognize, so it will be ignored`
),
})
})
})
@@ -0,0 +1,7 @@
export default function Page() {
return <p>hello world</p>
}

export const config = {
runtime: `something-${'real' + 1 + 'y odd'}`,
}