Skip to content

Commit

Permalink
warn on sync access if dynamicIO is not enabled (#71696)
Browse files Browse the repository at this point in the history
Since `console.error` is intercepted, this can be frustrating when a 3p
library that hasn't updated to use the async APIs triggers a warning.

This keeps the error behavior when `dynamicIO` is enabled and warns in
other cases.
  • Loading branch information
ztanner authored Oct 23, 2024
1 parent 741f830 commit bbad635
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,20 @@ const cache =
? React.cache
: (fn: (key: unknown) => void) => fn

// When Dynamic IO is enabled, we record these as errors so that they
// are captured by the dev overlay as it's more critical to fix these
// when enabled.
const logErrorOrWarn = process.env.__NEXT_DYNAMIC_IO
? console.error
: console.warn

// We don't want to dedupe across requests.
// The developer might've just attempted to fix the warning so we should warn again if it still happens.
const flushCurrentErrorIfNew = cache(
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- cache key
(key: unknown) => {
try {
console.error(errorRef.current)
logErrorOrWarn(errorRef.current)
} finally {
errorRef.current = null
}
Expand All @@ -41,7 +48,7 @@ export function createDedupedByCallsiteServerErrorLoggerDev<Args extends any[]>(
if (process.env.NODE_ENV !== 'production') {
const callStackFrames = new Error().stack?.split('\n')
if (callStackFrames === undefined || callStackFrames.length < 4) {
console.error(message)
logErrorOrWarn(message)
} else {
// Error:
// logDedupedError
Expand All @@ -53,7 +60,7 @@ export function createDedupedByCallsiteServerErrorLoggerDev<Args extends any[]>(
flushCurrentErrorIfNew(key)
}
} else {
console.error(message)
logErrorOrWarn(message)
}
}
}
2 changes: 1 addition & 1 deletion packages/next/src/server/request/cookies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import { getExpectedRequestStore } from '../app-render/work-unit-async-storage.external'
import { StaticGenBailoutError } from '../../client/components/static-generation-bailout'
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import { scheduleImmediate } from '../../lib/scheduler'

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/request/draft-mode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
postponeWithTracking,
trackSynchronousRequestDataAccessInDev,
} from '../app-render/dynamic-rendering'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import { StaticGenBailoutError } from '../../client/components/static-generation-bailout'
import { DynamicServerError } from '../../client/components/hooks-server-context'

Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/request/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from '../app-render/dynamic-rendering'
import { StaticGenBailoutError } from '../../client/components/static-generation-bailout'
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import { scheduleImmediate } from '../../lib/scheduler'

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/request/params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
import { InvariantError } from '../../shared/lib/invariant-error'
import { describeStringPropertyAccess, wellKnownProperties } from './utils'
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import { scheduleImmediate } from '../../lib/scheduler'

export type Params = Record<string, string | Array<string> | undefined>
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/server/request/search-params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
} from '../app-render/work-unit-async-storage.external'
import { InvariantError } from '../../shared/lib/invariant-error'
import { makeHangingPromise } from '../dynamic-rendering-utils'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-loger'
import { createDedupedByCallsiteServerErrorLoggerDev } from '../create-deduped-by-callsite-server-error-logger'
import {
describeStringPropertyAccess,
describeHasCheckingStringProperty,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ describe('dynamic-requests warnings', () => {
const browser = await next.browser('/request/cookies')

const browserLogs = await browser.log()
const browserConsoleErrors = browserLogs
.filter((log) => log.source === 'error')
const browserConsoleWarnings = browserLogs
.filter((log) => log.source === 'warning')
.map((log) => log.message)
const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex)
const terminalCookieErrors = terminalOutput.split('\n').filter((line) => {
return line.includes('Route "/request/cookies')
})
expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({
browserConsoleErrors: [
expect({ browserConsoleWarnings, terminalCookieErrors }).toEqual({
browserConsoleWarnings: [
expect.stringContaining("`cookies().get('page')`."),
expect.stringContaining("`cookies().get('component')`."),
expect.stringContaining("`cookies().has('component')`."),
Expand All @@ -39,16 +39,16 @@ describe('dynamic-requests warnings', () => {

const browser = await next.browser('/request/draftMode')

const browserLogsserLogs = await browser.log()
const browserConsoleErrors = browserLogsserLogs
.filter((log) => log.source === 'error')
const browserLogs = await browser.log()
const browserConsoleWarnings = browserLogs
.filter((log) => log.source === 'warning')
.map((log) => log.message)
const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex)
const terminalCookieErrors = terminalOutput.split('\n').filter((line) => {
return line.includes('Route "/request/draftMode')
})
expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({
browserConsoleErrors: [
expect({ browserConsoleWarnings, terminalCookieErrors }).toEqual({
browserConsoleWarnings: [
expect.stringContaining('`draftMode().isEnabled`.'),
expect.stringContaining('`draftMode().isEnabled`.'),
expect.stringContaining('`draftMode().enable()`.'),
Expand All @@ -68,16 +68,16 @@ describe('dynamic-requests warnings', () => {

const browser = await next.browser('/request/headers')

const browserLogsserLogs = await browser.log()
const browserConsoleErrors = browserLogsserLogs
.filter((log) => log.source === 'error')
const browserLogs = await browser.log()
const browserConsoleWarnings = browserLogs
.filter((log) => log.source === 'warning')
.map((log) => log.message)
const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex)
const terminalCookieErrors = terminalOutput.split('\n').filter((line) => {
return line.includes('Route "/request/headers')
})
expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({
browserConsoleErrors: [
expect({ browserConsoleWarnings, terminalCookieErrors }).toEqual({
browserConsoleWarnings: [
expect.stringContaining("`headers().get('page')`."),
expect.stringContaining("`headers().get('component')`."),
expect.stringContaining("`headers().has('component')`."),
Expand All @@ -97,16 +97,16 @@ describe('dynamic-requests warnings', () => {

const browser = await next.browser('/request/params/[slug]')

const browserLogsserLogs = await browser.log()
const browserConsoleErrors = browserLogsserLogs
.filter((log) => log.source === 'error')
const browserLogs = await browser.log()
const browserConsoleWarnings = browserLogs
.filter((log) => log.source === 'warning')
.map((log) => log.message)
const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex)
const terminalCookieErrors = terminalOutput.split('\n').filter((line) => {
return line.includes('Route "/request/params/[slug]')
})
expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({
browserConsoleErrors: [
expect({ browserConsoleWarnings, terminalCookieErrors }).toEqual({
browserConsoleWarnings: [
expect.stringContaining('`params.slug`.'),
expect.stringContaining('`params.slug`.'),
expect.stringContaining('`params.slug`.'),
Expand All @@ -126,16 +126,16 @@ describe('dynamic-requests warnings', () => {

const browser = await next.browser('/request/searchParams')

const browserLogsserLogs = await browser.log()
const browserConsoleErrors = browserLogsserLogs
.filter((log) => log.source === 'error')
const browserLogs = await browser.log()
const browserConsoleWarnings = browserLogs
.filter((log) => log.source === 'warning')
.map((log) => log.message)
const terminalOutput = next.cliOutput.slice(nextDevBootstrapOutputIndex)
const terminalCookieErrors = terminalOutput.split('\n').filter((line) => {
return line.includes('Route "/request/searchParams')
})
expect({ browserConsoleErrors, terminalCookieErrors }).toEqual({
browserConsoleErrors: [
expect({ browserConsoleWarnings, terminalCookieErrors }).toEqual({
browserConsoleWarnings: [
expect.stringContaining('`searchParams.slug`.'),
expect.stringContaining('`searchParams.slug`.'),
expect.stringContaining('`searchParams.slug`.'),
Expand All @@ -154,24 +154,24 @@ describe('dynamic-requests warnings', () => {
it('should have no warnings on normal rsc page without accessing params', async () => {
const browser = await next.browser('/no-access/normal')
const browserLogItems = await browser.log()
const browserConsoleErrors = browserLogItems
.filter((log) => log.source === 'error')
const browserConsoleWarnings = browserLogItems
.filter((log) => log.source === 'warning')
.map((log) => log.message)

expect(browserConsoleErrors.length).toBe(0)
expect(browserConsoleWarnings.length).toBe(0)
})

it('should only have hydration warnings on hydration mismatch page without accessing params', async () => {
const browser = await next.browser('/no-access/mismatch')
const browserLogItems = await browser.log()
console.log('browserLogItems', browserLogItems)
const browserConsoleErrors = browserLogItems
.filter((log) => log.source === 'error')
const browserConsoleWarnings = browserLogItems
.filter((log) => log.source === 'warning')
.map((log) => log.message)

// We assert there are zero logged errors but first we assert specific strings b/c we want a better
// failure message if these do appear
expect(browserConsoleErrors).toEqual(
expect(browserConsoleWarnings).toEqual(
expect.not.arrayContaining([
expect.stringContaining('param property was accessed directly with'),
expect.stringContaining(
Expand All @@ -182,7 +182,7 @@ describe('dynamic-requests warnings', () => {

// Even though there is a hydration error it does show up in the logs list b/c it is an
// uncaught Error not a console.error. We expect there to be no logged errors
expect(browserConsoleErrors.length).toBe(0)
expect(browserConsoleWarnings.length).toBe(0)
})
})
})

0 comments on commit bbad635

Please sign in to comment.