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

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

Merged
merged 8 commits into from
Mar 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af64caa
check
huozhi Feb 28, 2024
f184986
use rsc to check navigation client hook
huozhi Mar 11, 2024
2896d2b
update test
huozhi Mar 11, 2024
3cda308
update test
huozhi Mar 12, 2024
2900029
Add alias
huozhi Mar 12, 2024
5fb4f69
fix alias
huozhi Mar 12, 2024
f12c221
Merge branch 'canary' into swc/rsc-check-next-navigation
huozhi Mar 12, 2024
183f449
drop clientHookInServerComponentError
huozhi Mar 13, 2024
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
View file
Open in desktop
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
132 changes: 77 additions & 55 deletions packages/next-swc/crates/next-custom-transforms/src/transforms/react_server_components.rs
View file
Open in desktop
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
View file
Open in desktop
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'
32 changes: 13 additions & 19 deletions test/development/acceptance-app/server-components.test.ts
View file
Open in desktop
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 •
edited
Loading

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
Loading