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

Use SWC to valid client next/navigation hooks usage in server components #63160

Merged
merged 8 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/next-swc/crates/next-core/src/next_import_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,7 @@ async fn rsc_aliases(
"react-server-dom-webpack/server.node" => format!("next/dist/server/future/route-modules/app-page/vendored/rsc/react-server-dom-turbopack-server-node"),
"react-server-dom-turbopack/server.edge" => format!("next/dist/server/future/route-modules/app-page/vendored/rsc/react-server-dom-turbopack-server-edge"),
"react-server-dom-turbopack/server.node" => format!("next/dist/server/future/route-modules/app-page/vendored/rsc/react-server-dom-turbopack-server-node"),
"next/navigation" => format!("next/dist/api/navigation.react-server"),

// Needed to make `react-dom/server` work.
"next/dist/compiled/react" => format!("next/dist/compiled/react/index.js"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,8 +473,7 @@ struct ReactServerComponentValidator {
app_dir: Option<PathBuf>,
invalid_server_imports: Vec<JsWord>,
invalid_client_imports: Vec<JsWord>,
invalid_server_react_apis: Vec<JsWord>,
invalid_server_react_dom_apis: Vec<JsWord>,
invalid_server_lib_apis_mapping: HashMap<&'static str, Vec<&'static str>>,
pub directive_import_collection: Option<(bool, bool, Vec<ModuleImports>, Vec<String>)>,
}

Expand All @@ -485,86 +484,109 @@ impl ReactServerComponentValidator {
filepath: filename,
app_dir,
directive_import_collection: None,
// react -> [apis]
// react-dom -> [apis]
// next/navigation -> [apis]
invalid_server_lib_apis_mapping: [
(
"react",
vec![
"Component",
"createContext",
"createFactory",
"PureComponent",
"useDeferredValue",
"useEffect",
"useImperativeHandle",
"useInsertionEffect",
"useLayoutEffect",
"useReducer",
"useRef",
"useState",
"useSyncExternalStore",
"useTransition",
"useOptimistic",
],
),
(
"react-dom",
vec![
"findDOMNode",
"flushSync",
"unstable_batchedUpdates",
"useFormStatus",
"useFormState",
],
),
(
"next/navigation",
vec![
"useSearchParams",
"usePathname",
"useSelectedLayoutSegment",
"useSelectedLayoutSegments",
"useParams",
"useRouter",
"useServerInsertedHTML",
"ServerInsertedHTMLContext",
],
),
]
.into(),

invalid_server_imports: vec![
JsWord::from("client-only"),
JsWord::from("react-dom/client"),
JsWord::from("react-dom/server"),
JsWord::from("next/router"),
],
invalid_client_imports: vec![JsWord::from("server-only"), JsWord::from("next/headers")],
invalid_server_react_dom_apis: vec![
JsWord::from("findDOMNode"),
JsWord::from("flushSync"),
JsWord::from("unstable_batchedUpdates"),
JsWord::from("useFormStatus"),
JsWord::from("useFormState"),
],
invalid_server_react_apis: vec![
JsWord::from("Component"),
JsWord::from("createContext"),
JsWord::from("createFactory"),
JsWord::from("PureComponent"),
JsWord::from("useDeferredValue"),
JsWord::from("useEffect"),
JsWord::from("useImperativeHandle"),
JsWord::from("useInsertionEffect"),
JsWord::from("useLayoutEffect"),
JsWord::from("useReducer"),
JsWord::from("useRef"),
JsWord::from("useState"),
JsWord::from("useSyncExternalStore"),
JsWord::from("useTransition"),
JsWord::from("useOptimistic"),
],
}
}

fn is_from_node_modules(&self, filepath: &str) -> bool {
Regex::new(r"node_modules[\\/]").unwrap().is_match(filepath)
}

// Asserts the server lib apis
// e.g.
// assert_invalid_server_lib_apis("react", import)
// assert_invalid_server_lib_apis("react-dom", import)
fn assert_invalid_server_lib_apis(&self, import_source: String, import: &ModuleImports) {
// keys of invalid_server_lib_apis_mapping
let invalid_apis = self
.invalid_server_lib_apis_mapping
.get(import_source.as_str());
if let Some(invalid_apis) = invalid_apis {
for specifier in &import.specifiers {
if invalid_apis.contains(&specifier.0.as_str()) {
report_error(
&self.app_dir,
&self.filepath,
RSCErrorKind::NextRscErrReactApi((specifier.0.to_string(), specifier.1)),
);
}
}
}
}

fn assert_server_graph(&self, imports: &[ModuleImports], module: &Module) {
// If the
if self.is_from_node_modules(&self.filepath) {
return;
}
for import in imports {
let source = import.source.0.clone();
let source_str = source.to_string();
if self.invalid_server_imports.contains(&source) {
report_error(
&self.app_dir,
&self.filepath,
RSCErrorKind::NextRscErrServerImport((source.to_string(), import.source.1)),
RSCErrorKind::NextRscErrServerImport((source_str.clone(), import.source.1)),
);
}
if source == *"react" {
for specifier in &import.specifiers {
if self.invalid_server_react_apis.contains(&specifier.0) {
report_error(
&self.app_dir,
&self.filepath,
RSCErrorKind::NextRscErrReactApi((
specifier.0.to_string(),
specifier.1,
)),
);
}
}
}
if source == *"react-dom" {
for specifier in &import.specifiers {
if self.invalid_server_react_dom_apis.contains(&specifier.0) {
report_error(
&self.app_dir,
&self.filepath,
RSCErrorKind::NextRscErrReactApi((
specifier.0.to_string(),
specifier.1,
)),
);
}
}
}

self.assert_invalid_server_lib_apis(source_str, import);
}

self.assert_invalid_api(module, false);
Expand Down
27 changes: 0 additions & 27 deletions packages/next/src/api/navigation.react-server.ts
Original file line number Diff line number Diff line change
@@ -1,28 +1 @@
import { errorClientApi } from '../client/components/client-hook-in-server-component-error'

export function useSearchParams() {
errorClientApi('useSearchParams')
}
export function usePathname() {
errorClientApi('usePathname')
}
export function useSelectedLayoutSegment() {
errorClientApi('useSelectedLayoutSegment')
}
export function useSelectedLayoutSegments() {
errorClientApi('useSelectedLayoutSegments')
}
export function useParams() {
errorClientApi('useParams')
}
export function useRouter() {
errorClientApi('useRouter')
}
export function useServerInsertedHTML() {
errorClientApi('useServerInsertedHTML')
}
export function ServerInsertedHTMLContext() {
errorClientApi('ServerInsertedHTMLContext')
}

export * from '../client/components/navigation.react-server'

This file was deleted.

8 changes: 0 additions & 8 deletions packages/next/src/client/components/navigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
PathnameContext,
PathParamsContext,
} from '../../shared/lib/hooks-client-context.shared-runtime'
import { clientHookInServerComponentError } from './client-hook-in-server-component-error'
import { getSegmentValue } from './router-reducer/reducers/get-segment-value'
import { PAGE_SEGMENT_KEY, DEFAULT_SEGMENT_KEY } from '../../shared/lib/segment'
import { ReadonlyURLSearchParams } from './navigation.react-server'
Expand All @@ -36,7 +35,6 @@ import { ReadonlyURLSearchParams } from './navigation.react-server'
* Read more: [Next.js Docs: `useSearchParams`](https://nextjs.org/docs/app/api-reference/functions/use-search-params)
*/
function useSearchParams(): ReadonlyURLSearchParams {
clientHookInServerComponentError('useSearchParams')
const searchParams = useContext(SearchParamsContext)

// In the case where this is `null`, the compat types added in
Expand Down Expand Up @@ -81,7 +79,6 @@ function useSearchParams(): ReadonlyURLSearchParams {
* Read more: [Next.js Docs: `usePathname`](https://nextjs.org/docs/app/api-reference/functions/use-pathname)
*/
function usePathname(): string {
clientHookInServerComponentError('usePathname')
// In the case where this is `null`, the compat types added in `next-env.d.ts`
// will add a new overload that changes the return type to include `null`.
return useContext(PathnameContext) as string
Expand Down Expand Up @@ -111,7 +108,6 @@ import {
* Read more: [Next.js Docs: `useRouter`](https://nextjs.org/docs/app/api-reference/functions/use-router)
*/
function useRouter(): AppRouterInstance {
clientHookInServerComponentError('useRouter')
const router = useContext(AppRouterContext)
if (router === null) {
throw new Error('invariant expected app router to be mounted')
Expand Down Expand Up @@ -142,8 +138,6 @@ interface Params {
* Read more: [Next.js Docs: `useParams`](https://nextjs.org/docs/app/api-reference/functions/use-params)
*/
function useParams<T extends Params = Params>(): T {
clientHookInServerComponentError('useParams')

return useContext(PathParamsContext) as T
}

Expand Down Expand Up @@ -210,7 +204,6 @@ function getSelectedLayoutSegmentPath(
function useSelectedLayoutSegments(
parallelRouteKey: string = 'children'
): string[] {
clientHookInServerComponentError('useSelectedLayoutSegments')
const context = useContext(LayoutRouterContext)
// @ts-expect-error This only happens in `pages`. Type is overwritten in navigation.d.ts
if (!context) return null
Expand Down Expand Up @@ -239,7 +232,6 @@ function useSelectedLayoutSegments(
function useSelectedLayoutSegment(
parallelRouteKey: string = 'children'
): string | null {
clientHookInServerComponentError('useSelectedLayoutSegment')
const selectedLayoutSegments = useSelectedLayoutSegments(parallelRouteKey)

if (!selectedLayoutSegments || selectedLayoutSegments.length === 0) {
Expand Down
32 changes: 13 additions & 19 deletions test/development/acceptance-app/server-components.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,45 +521,39 @@ describe('Error Overlay for server components', () => {
})
})

describe('Next.js component hooks called in Server Component', () => {
describe('Next.js navigation client hooks called in Server Component', () => {
it.each([
// TODO-APP: add test for useParams
// ["useParams"],
['useParams'],
['useRouter'],
['usePathname'],
['useSearchParams'],
['useSelectedLayoutSegment'],
['useSelectedLayoutSegments'],
['usePathname'],
])('should show error when %s is called', async (hook: string) => {
const { browser, cleanup } = await sandbox(
const { session, cleanup } = await sandbox(
next,
new Map([
[
'app/page.js',
outdent`
import { ${hook} } from 'next/navigation'
export default function Page() {
${hook}()
return "Hello world"
}
`,
],
])
)

await check(async () => {
expect(
await browser
.waitForElementByCss('#nextjs__container_errors_desc')
.text()
).toContain(
`Error: ${hook} only works in Client Components. Add the "use client" directive at the top of the file to use it. Read more: https://nextjs.org/docs/messages/react-client-hook-in-server-component`
)
return 'success'
}, 'success')

expect(next.cliOutput).toContain(
`Error: ${hook} only works in Client Components. Add the "use client" directive at the top of the file to use it. Read more: https://nextjs.org/docs/messages/react-client-hook-in-server-component`
expect(await session.hasRedbox()).toBe(true)
// In webpack when the message too long it gets truncated with ` | ` with new lines.
// So we need to check for the first part of the message.
const normalizedSource = await session.getRedboxSource()
expect(normalizedSource).toContain(
`You're importing a component that needs ${hook}. It only works in a Client Component but none of its parents are marked with "use client"`
Copy link
Contributor

@gnoff gnoff Mar 12, 2024

Choose a reason for hiding this comment

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

You didn't change this string so it's probably a long standing message but I wonder if we should change this. First, you may not be importing a component at all. You may be importing some random value from a file which just happens to be importing this hook. Maybe something like this would be more true to what is happening

${pathToFileContainingClientOnlyImport} is being imported in a Server Component context by ${pathToFileDoingTheImporting} but it imports a client only ${ClientOnlyImport} from ${package specifier}. If you intended to only use this file in a Client Component context ensure that it or at least one of it's parent modules is marked with "use client". If you want to use one or more exports in a Server Component context extract them into their own module that does not depend on any client only imports from ${package specifier}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to improve that as well, we have the similar error message like Maybe one of these should be marked as a client entry 'use client' and printing the module trace, mainly for React client hooks before. I can improve this in a separate PR 👍

)
expect(normalizedSource).toContain(
`import { ${hook} } from 'next/navigation'`
)

await cleanup()
Expand Down