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.

Sign up for GitHub

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

Jump to bottom

fix(next): improve error for using <Html> outside of document #45056

Merged
merged 5 commits into from Jun 28, 2023
Merged

fix(next): improve error for using <Html> outside of document #45056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
21e8366
fix(next): improve error for using <Html> outside of document
JoshuaKGoldberg Jan 19, 2023
d806849
Merge branch 'canary' into safe-html-context
JoshuaKGoldberg Jun 20, 2023
2ab53d9
Merge branch 'canary' into safe-html-context
JoshuaKGoldberg Jun 27, 2023
76cfe99
Merge branch 'canary' into safe-html-context
ijjk Jun 28, 2023
c31f2cb
Merge branch 'canary' into safe-html-context
kodiakhq[bot] Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/next/src/pages/_document.tsx
View file
Open in desktop
@@ -1,4 +1,4 @@
import React, { ReactElement, ReactNode, useContext } from 'react'
import React, { ReactElement, ReactNode } from 'react'
import {
OPTIMIZED_FONT_PROVIDERS,
NEXT_BUILTIN_DOCUMENT,
Expand All @@ -17,7 +17,7 @@ import { BuildManifest, getPageFiles } from '../server/get-page-files'
import { htmlEscapeJsonString } from '../server/htmlescape'
import isError from '../lib/is-error'

import { HtmlContext } from '../shared/lib/html-context'
import { HtmlContext, useHtmlContext } from '../shared/lib/html-context'
import type { HtmlProps } from '../shared/lib/html-context'

export { DocumentContext, DocumentInitialProps, DocumentProps }
Expand Down Expand Up @@ -411,7 +411,7 @@ function getFontLoaderLinks(
export class Head extends React.Component<HeadProps> {
static contextType = HtmlContext

context!: React.ContextType<typeof HtmlContext>
context!: HtmlProps

getCssLinks(files: DocumentFiles): JSX.Element[] | null {
const {
Expand Down Expand Up @@ -968,7 +968,7 @@ function handleDocumentScriptLoaderItems(
export class NextScript extends React.Component<OriginProps> {
static contextType = HtmlContext

context!: React.ContextType<typeof HtmlContext>
context!: HtmlProps

getDynamicChunks(files: DocumentFiles) {
return getDynamicChunks(this.context, this.props, files)
Expand Down Expand Up @@ -1138,7 +1138,7 @@ export function Html(
locale,
scriptLoader,
__NEXT_DATA__,
} = useContext(HtmlContext)
} = useHtmlContext()

docComponentsRendered.Html = true
handleDocumentScriptLoaderItems(scriptLoader, __NEXT_DATA__, props)
Expand All @@ -1160,7 +1160,7 @@ export function Html(
}

export function Main() {
const { docComponentsRendered } = useContext(HtmlContext)
const { docComponentsRendered } = useHtmlContext()
docComponentsRendered.Main = true
// @ts-ignore
return <next-js-internal-body-render-target />
Expand Down
17 changes: 15 additions & 2 deletions packages/next/src/shared/lib/html-context.ts
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ import type { NEXT_DATA } from './utils'
import type { FontConfig } from '../../server/font-utils'
import type { FontLoaderManifest } from '../../build/webpack/plugins/font-loader-manifest-plugin'

import { createContext } from 'react'
import { createContext, useContext } from 'react'

export type HtmlProps = {
__NEXT_DATA__: NEXT_DATA
Expand Down Expand Up @@ -46,7 +46,20 @@ export type HtmlProps = {
fontLoaderManifest?: FontLoaderManifest
}

export const HtmlContext = createContext<HtmlProps>(null as any)
export const HtmlContext = createContext<HtmlProps | undefined>(undefined)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yet another case where as any is a Bad 😈

Explaining: createContext is a hard API to get type safe. Its typings indicate that you must provide a default value for it: that way, when it's called, it's known to always return a value. (In more technical terms, createContext<T> takes in a parameter of type T so the return type is T, not T | undefined.)

Unfortunately that means that when there isn't a reasonable default value -the majority of the time in my experience 🙃- projects tend to provide something bogus like null as any to get around type checking. They then assume useContext will only be called with that context in places where someone has used that context's provider to provide a real value.

The assumption that someone's provided an overridden value isn't always correct! We see in this PR's linked bug #45024 that if someone renders <Html> outside of pages/_document.(js|tsx), Next.js hasn't provided a context value with HtmlContext.Provider. So when code tries const { ... } = useContext(HtmlContext), that evaluates to const { ... } = undefined, which is a runtime error.

This PR doesn't change whether an error is thrown. It just throws a nicer error for users to read.

if (process.env.NODE_ENV !== 'production') {
HtmlContext.displayName = 'HtmlContext'
}

export function useHtmlContext() {
const context = useContext(HtmlContext)

if (!context) {
throw new Error(
`<Html> should not be imported outside of pages/_document.\n` +
'Read more: https://nextjs.org/docs/messages/no-document-import-in-page'
)
}

return context
}