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

feat: Introduce eslint rule for async client components #51547

Merged
merged 13 commits into from
Jun 26, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ Next.js provides an ESLint plugin, [`eslint-plugin-next`](https://www.npmjs.com/
| <Check size={18} /> | [@next/next/inline-script-id](/docs/messages/inline-script-id) | Enforce `id` attribute on `next/script` components with inline content. |
| <Check size={18} /> | [@next/next/next-script-for-ga](/docs/messages/next-script-for-ga) | Prefer `next/script` component when using the inline script for Google Analytics. |
| <Check size={18} /> | [@next/next/no-assign-module-variable](/docs/messages/no-assign-module-variable) | Prevent assignment to the `module` variable. |
| <Check size={18} /> | [@next/next/no-async-client-component](/docs/messages/no-async-client-component) | Prevent client components from being async functions. |
| <Check size={18} /> | [@next/next/no-before-interactive-script-outside-document](/docs/messages/no-before-interactive-script-outside-document) | Prevent usage of `next/script`'s `beforeInteractive` strategy outside of `pages/_document.js`. |
| <Check size={18} /> | [@next/next/no-css-tags](/docs/messages/no-css-tags) | Prevent manual stylesheet tags. |
| <Check size={18} /> | [@next/next/no-document-import-in-page](/docs/messages/no-document-import-in-page) | Prevent importing `next/document` outside of `pages/_document.js`. |
Expand Down
12 changes: 12 additions & 0 deletions errors/no-async-client-component.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# No async client component

> Client components cannot be async functions.

#### Why This Error Occurred

As per the [React Server Component RFC on promise support](https://github.com/acdlite/rfcs/blob/first-class-promises/text/0000-first-class-support-for-promises.md), [client components cannot be async functions](https://github.com/acdlite/rfcs/blob/first-class-promises/text/0000-first-class-support-for-promises.md#why-cant-client-components-be-async-functions).

#### Possible Ways to Fix It

1. Remove the `async` keyword from the client component function declaration, or
2. Convert the client component to a server component
2 changes: 2 additions & 0 deletions packages/eslint-plugin-next/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module.exports = {
'inline-script-id': require('./rules/inline-script-id'),
'next-script-for-ga': require('./rules/next-script-for-ga'),
'no-assign-module-variable': require('./rules/no-assign-module-variable'),
'no-async-client-component': require('./rules/no-async-client-component'),
'no-before-interactive-script-outside-document': require('./rules/no-before-interactive-script-outside-document'),
'no-css-tags': require('./rules/no-css-tags'),
'no-document-import-in-page': require('./rules/no-document-import-in-page'),
Expand Down Expand Up @@ -43,6 +44,7 @@ module.exports = {
// errors
'@next/next/inline-script-id': 'error',
'@next/next/no-assign-module-variable': 'error',
'@next/next/no-async-client-component': 'error',
'@next/next/no-document-import-in-page': 'error',
'@next/next/no-duplicate-head': 'error',
'@next/next/no-head-import-in-document': 'error',
Expand Down
68 changes: 68 additions & 0 deletions packages/eslint-plugin-next/src/rules/no-async-client-component.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { defineRule } from '../utils/define-rule'

const url = 'https://nextjs.org/docs/messages/no-async-client-component'
const description = 'Prevent client components from being async functions.'
const message = `${description} See: ${url}`

export = defineRule({
meta: {
docs: {
description,
recommended: true,
url,
},
type: 'problem',
schema: [],
},

create(context) {
return {
Program(node) {
let isClientComponent: boolean = false

for (const block of node.body) {
if (
block.type === 'ExpressionStatement' &&
block.expression.type === 'Literal' &&
block.expression.value === 'use client'
Copy link
Member

Choose a reason for hiding this comment

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

The tricky thing about this being a linter rule is that, even if a file doesn't have "use client" on top, it can still be a Client Component (e.g. it can be imported by a file with "use client"). And also, a file with "use client" can still export non-component functions that are async. Not strongly against adding such a linter rule, but we can probably make it a warning instead of an error, and ensure that the exported function's name start with a capitalized character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review @shuding! I pushed the changes you suggested.

Agree this as a linter rule isn't a complete solution. In addition to the case you mentioned it also does not handle anonymous default exports or named exports. It is low-hanging fruit that should help prevent the page crashes people are reporting in #50898 and #50382, at least for ESLint users using Next's recommended config.

Linter rule aside, is it possible to throw an error at runtime for this scenario? That way it it avoids the page crash and provides some recourse to developers in the error message. As it stands right now there is no indication why the page crash happens

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Yeah it sounds good to have this shipped first, at the same time we'll check how this can be improved over time.

Unfortunately we can't add this as a runtime check. We can't abort synchronous operations in that case.

) {
isClientComponent = true
}

if (block.type === 'ExportDefaultDeclaration' && isClientComponent) {
// export default async function MyComponent() {...}
if (
block.declaration.type === 'FunctionDeclaration' &&
block.declaration.async
) {
context.report({
node: block,
message,
})
}

// async function MyComponent() {...}; export default MyComponent;
if (block.declaration.type === 'Identifier') {
const functionName = block.declaration.name
const functionDeclaration = node.body.find(
(localBlock) =>
localBlock.type === 'FunctionDeclaration' &&
localBlock.id.name === functionName
)

if (
functionDeclaration.type === 'FunctionDeclaration' &&
functionDeclaration.async
) {
context.report({
node: functionDeclaration,
message,
})
}
}
}
}
},
}
},
})
72 changes: 72 additions & 0 deletions test/unit/eslint-plugin-next/no-async-client-component.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import rule from '@next/eslint-plugin-next/dist/rules/no-async-client-component'
import { RuleTester } from 'eslint'
;(RuleTester as any).setDefaultConfig({
parserOptions: {
ecmaVersion: 2018,
sourceType: 'module',
ecmaFeatures: {
modules: true,
jsx: true,
},
},
})
const ruleTester = new RuleTester()

const message =
'Prevent client components from being async functions. See: https://nextjs.org/docs/messages/no-async-client-component'

ruleTester.run('no-async-client-component single line', rule, {
valid: [
`
export default async function MyComponent() {
return <></>
}
`,
],
invalid: [
{
code: `
"use client"

export default async function MyComponent() {
return <></>
}
`,
errors: [
{
message,
},
],
},
],
})

ruleTester.run('no-async-client-component multiple line', rule, {
valid: [
`
async function MyComponent() {
return <></>
}

export default MyComponent
`,
],
invalid: [
{
code: `
"use client"

async function MyComponent() {
return <></>
}

export default MyComponent
`,
errors: [
{
message,
},
],
},
],
})