From 5e74aecd15c9f641afcf873aa4ebf2ecf6bed9a1 Mon Sep 17 00:00:00 2001 From: kongjiacong Date: Sat, 11 Jan 2025 14:28:46 +0800 Subject: [PATCH 1/3] fix: unified loader response logic --- .../src/cli/createRequest.ts | 71 ++++++++++---- .../plugin-data-loader/src/runtime/errors.ts | 2 +- .../plugin-data-loader/src/runtime/index.ts | 2 +- .../src/router/runtime/plugin.node.tsx | 9 +- .../three/routes/error/response/page.data.ts | 43 ++++++-- .../src/three/routes/error/response/page.tsx | 8 +- tests/integration/routes/tests/index.test.ts | 97 ++++++++++++++++--- 7 files changed, 180 insertions(+), 52 deletions(-) diff --git a/packages/cli/plugin-data-loader/src/cli/createRequest.ts b/packages/cli/plugin-data-loader/src/cli/createRequest.ts index 9b567815965d..6e659069f3db 100644 --- a/packages/cli/plugin-data-loader/src/cli/createRequest.ts +++ b/packages/cli/plugin-data-loader/src/cli/createRequest.ts @@ -38,28 +38,45 @@ const handleRedirectResponse = (res: Response) => { return res; }; -const handleDeferredResponse = async (res: Response) => { - if ( - res.headers.get('Content-Type')?.match(CONTENT_TYPE_DEFERRED) && - res.body - ) { - return await parseDeferredReadableStream(res.body); - } - return res; +const isDeferredResponse = (res: Response) => { + return ( + res.headers.get('Content-Type')?.match(CONTENT_TYPE_DEFERRED) && res.body + ); +}; + +const isRedirectResponse = (res: Response) => { + return res.headers.get('X-Modernjs-Redirect') != null; }; const isErrorResponse = (res: Response) => { return res.headers.get('X-Modernjs-Error') != null; }; +function isOtherErrorResponse(res: Response) { + return ( + res.status >= 400 && + res.headers.get('X-Modernjs-Error') == null && + res.headers.get('X-Modernjs-Catch') == null && + res.headers.get('X-Modernjs-Response') == null + ); +} + +const isCatchResponse = (res: Response) => { + return res.headers.get('X-Modernjs-Catch') != null; +}; + const handleErrorResponse = async (res: Response) => { - if (isErrorResponse(res)) { - const data = await res.json(); - const error = new Error(data.message); - error.stack = data.stack; - throw error; - } - return res; + const data = await res.json(); + const error = new Error(data.message); + error.stack = data.stack; + throw error; +}; + +const handleNetworkErrorResponse = async (res: Response) => { + const text = await res.text(); + const error = new Error(text); + error.stack = undefined; + throw error; }; export const createRequest = (routeId: string, method = 'get') => { @@ -75,11 +92,29 @@ export const createRequest = (routeId: string, method = 'get') => { res = await fetch(url, { method, signal: request.signal, + }).catch(error => { + throw error; }); - res = handleRedirectResponse(res); - res = await handleErrorResponse(res); - res = await handleDeferredResponse(res); + if (isRedirectResponse(res)) { + return handleRedirectResponse(res); + } + + if (isErrorResponse(res)) { + return await handleErrorResponse(res); + } + + if (isCatchResponse(res)) { + throw res; + } + + if (isDeferredResponse(res)) { + return await parseDeferredReadableStream(res.body!); + } + + if (isOtherErrorResponse(res)) { + return await handleNetworkErrorResponse(res); + } return res; }; diff --git a/packages/cli/plugin-data-loader/src/runtime/errors.ts b/packages/cli/plugin-data-loader/src/runtime/errors.ts index 92b23c03950d..07cba8c9fd6a 100644 --- a/packages/cli/plugin-data-loader/src/runtime/errors.ts +++ b/packages/cli/plugin-data-loader/src/runtime/errors.ts @@ -65,7 +65,7 @@ export function sanitizeError(error: T) { process.env.NODE_ENV !== 'development' && process.env.NODE_ENV !== 'test' ) { - const sanitized = new Error('Unexpected Server Error'); + const sanitized = new Error(error.message || 'Unexpected Server Error'); sanitized.stack = undefined; return sanitized; } diff --git a/packages/cli/plugin-data-loader/src/runtime/index.ts b/packages/cli/plugin-data-loader/src/runtime/index.ts index 9409ac1050f8..10705b0b7b6b 100644 --- a/packages/cli/plugin-data-loader/src/runtime/index.ts +++ b/packages/cli/plugin-data-loader/src/runtime/index.ts @@ -130,7 +130,7 @@ export const handleRequest = async ({ }); } const cost = end(); - + response.headers.set('X-Modernjs-Response', 'yes'); onTiming?.(`${LOADER_REPORTER_NAME}-navigation`, cost); } catch (error) { if (isResponse(error)) { diff --git a/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx b/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx index 73e100e40751..68d88f91eb7e 100644 --- a/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx +++ b/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx @@ -127,16 +127,15 @@ export const routerPlugin = ( // requestHandler.ts also has same logic, but handle common status code // maybe we can put the logic in requestHandler.ts in the future + const errors = Object.values( + (routerContext.errors || {}) as Record, + ); if ( - routerContext.statusCode >= 500 && - routerContext.statusCode < 600 && // TODO: if loaderFailureMode is not 'errroBoundary', error log will not be printed. + errors.length > 0 && loaderFailureMode === 'clientRender' ) { routerContext.statusCode = 200; - const errors = Object.values( - routerContext.errors as Record, - ); throw errors[0]; } diff --git a/tests/integration/routes/src/three/routes/error/response/page.data.ts b/tests/integration/routes/src/three/routes/error/response/page.data.ts index 4c4f95d6fdf4..629bc634d6d5 100644 --- a/tests/integration/routes/src/three/routes/error/response/page.data.ts +++ b/tests/integration/routes/src/three/routes/error/response/page.data.ts @@ -1,10 +1,35 @@ -export const loader = () => { - throw new Response( - JSON.stringify({ - message: "can't found the user", - }), - { - status: 255, - }, - ); +import type { LoaderFunctionArgs } from '@modern-js/runtime/router'; + +export const loader = ({ request }: LoaderFunctionArgs) => { + const url = new URL(request.url); + const responseType = url.searchParams.get('type') as string; + const statusCode = url.searchParams.get('code') as string; + + if (responseType === 'throw_error') { + throw new Error("can't found the user"); + } + + if (responseType === 'throw_response') { + throw new Response( + JSON.stringify({ + message: "can't found the user", + }), + { + status: Number(statusCode), + }, + ); + } + + if (responseType === 'return_response') { + return new Response( + JSON.stringify({ + message: 'user modern.js', + }), + { + status: Number(statusCode), + }, + ); + } + + return null; }; diff --git a/tests/integration/routes/src/three/routes/error/response/page.tsx b/tests/integration/routes/src/three/routes/error/response/page.tsx index c478cee9e008..6fd638f7448a 100644 --- a/tests/integration/routes/src/three/routes/error/response/page.tsx +++ b/tests/integration/routes/src/three/routes/error/response/page.tsx @@ -1,10 +1,6 @@ -import { Outlet, useLoaderData } from '@modern-js/runtime/router'; +import { useLoaderData } from '@modern-js/runtime/router'; export default function Page() { const data = useLoaderData(); - return ( -
- -
- ); + return
Response Page
; } diff --git a/tests/integration/routes/tests/index.test.ts b/tests/integration/routes/tests/index.test.ts index cff6d96ead5e..77e6b0c582f0 100644 --- a/tests/integration/routes/tests/index.test.ts +++ b/tests/integration/routes/tests/index.test.ts @@ -1,5 +1,5 @@ import path from 'path'; -import { fs, ROUTE_MANIFEST_FILE } from '@modern-js/utils'; +import { fs, ROUTE_MANIFEST_FILE, wait } from '@modern-js/utils'; import { ROUTE_MANIFEST } from '@modern-js/utils/universal/constants'; import puppeteer, { type Browser } from 'puppeteer'; @@ -11,6 +11,7 @@ import { launchOptions, modernBuild, modernServe, + sleep, } from '../../../utils/modernTestUtils'; const appDir = path.resolve(__dirname, '../'); @@ -361,27 +362,67 @@ const supportLoader = async (page: Page, errors: string[], appPort: number) => { expect(errors.length).toBe(0); }; +const supportThrowError = async ( + page: Page, + errors: string[], + appPort: number, +) => { + const response = await page.goto( + `http://localhost:${appPort}/three/error/response?type=throw_error`, + { + waitUntil: ['domcontentloaded'], + }, + ); + expect(response?.status()).toBe(200); + await page.waitForSelector('.error-content'); + const errorStatusElm = await page.$('.error-content'); + const text = await page.evaluate(el => el?.textContent, errorStatusElm); + expect(text?.includes('500')).toBeFalsy(); + expect(text?.includes("can't found the user")).toBeTruthy(); +}; + const supportThrowResponse = async ( page: Page, errors: string[], appPort: number, + code: number, ) => { const response = await page.goto( - `http://localhost:${appPort}/three/error/response`, + `http://localhost:${appPort}/three/error/response?type=throw_response&code=${code}`, { waitUntil: ['domcontentloaded'], }, ); - expect(response?.status()).toBe(255); + expect(response?.status()).toBe(200); + await page.waitForSelector('.response-status'); const errorStatusElm = await page.$('.response-status'); const text = await page.evaluate(el => el?.textContent, errorStatusElm); - expect(text?.includes('255')).toBeTruthy(); + expect(text?.includes(`${code}`)).toBeTruthy(); const errorContentElm = await page.$('.response-content'); const text1 = await page.evaluate(el => el?.textContent, errorContentElm); expect(text1?.includes("can't found the user")).toBeTruthy(); expect(errors.length).toBe(0); }; +const supportReturnResponse = async ( + page: Page, + errors: string[], + appPort: number, + code: number, +) => { + const response = await page.goto( + `http://localhost:${appPort}/three/error/response?type=return_response&code=${code}`, + { + waitUntil: ['domcontentloaded'], + }, + ); + expect(response?.status()).toBe(code); + await page.waitForSelector('.response-content'); + const el = await page.$('.response-content'); + const text = await page.evaluate(el => el?.textContent, el); + expect(text?.includes('Response Page')).toBeTruthy(); +}; + const supportLoaderForSSRAndCSR = async ( page: Page, errors: string[], @@ -770,8 +811,16 @@ describe('dev', () => { supportRedirectForSSR(page, errors, appPort)); test('support redirect for csr', () => supportRedirectForCSR(page, errors, appPort)); - test('support throw response', async () => - supportThrowResponse(page, errors, appPort)); + test('support throw error', async () => + supportThrowError(page, errors, appPort)); + test('support throw response', async () => { + await supportThrowResponse(page, errors, appPort, 500); + await supportThrowResponse(page, errors, appPort, 200); + }); + test('support return response', async () => { + await supportReturnResponse(page, errors, appPort, 500); + await supportReturnResponse(page, errors, appPort, 200); + }); }); describe('global configuration', () => { @@ -912,8 +961,16 @@ describe('build', () => { supportRedirectForSSR(page, errors, appPort)); test('support redirect for csr', () => supportRedirectForCSR(page, errors, appPort)); - test('support throw response', async () => - supportThrowResponse(page, errors, appPort)); + test('support throw error', async () => + supportThrowError(page, errors, appPort)); + test('support throw response', async () => { + await supportThrowResponse(page, errors, appPort, 500); + await supportThrowResponse(page, errors, appPort, 200); + }); + test('support return response', async () => { + await supportReturnResponse(page, errors, appPort, 500); + await supportReturnResponse(page, errors, appPort, 200); + }); }); describe('global configuration', () => { @@ -1056,8 +1113,16 @@ describe('dev with rspack', () => { supportRedirectForSSR(page, errors, appPort)); test('support redirect for csr', () => supportRedirectForCSR(page, errors, appPort)); - test('support throw response', async () => - supportThrowResponse(page, errors, appPort)); + test('support throw error', async () => + supportThrowError(page, errors, appPort)); + test('support throw response', async () => { + await supportThrowResponse(page, errors, appPort, 500); + await supportThrowResponse(page, errors, appPort, 200); + }); + test('support return response', async () => { + await supportReturnResponse(page, errors, appPort, 500); + await supportReturnResponse(page, errors, appPort, 200); + }); }); describe('global configuration', () => { @@ -1202,8 +1267,16 @@ describe('build with rspack', () => { supportRedirectForSSR(page, errors, appPort)); test('support redirect for csr', () => supportRedirectForCSR(page, errors, appPort)); - test('support throw response', async () => - supportThrowResponse(page, errors, appPort)); + test('support throw error', async () => + supportThrowError(page, errors, appPort)); + test('support throw response', async () => { + await supportThrowResponse(page, errors, appPort, 500); + await supportThrowResponse(page, errors, appPort, 200); + }); + test('support return response', async () => { + await supportReturnResponse(page, errors, appPort, 500); + await supportReturnResponse(page, errors, appPort, 200); + }); }); describe('global configuration', () => { From 782f73ee2b5d39a69255d0a8ca3e845d502a0e4f Mon Sep 17 00:00:00 2001 From: kongjiacong Date: Sat, 11 Jan 2025 14:38:44 +0800 Subject: [PATCH 2/3] chore: cr --- packages/cli/plugin-data-loader/src/cli/createRequest.ts | 1 + packages/cli/plugin-data-loader/src/runtime/index.ts | 1 + .../runtime/plugin-runtime/src/core/server/requestHandler.ts | 2 +- .../runtime/plugin-runtime/src/router/runtime/plugin.node.tsx | 4 ++-- tests/integration/routes/tests/index.test.ts | 3 +-- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/cli/plugin-data-loader/src/cli/createRequest.ts b/packages/cli/plugin-data-loader/src/cli/createRequest.ts index 6e659069f3db..5b90a620358d 100644 --- a/packages/cli/plugin-data-loader/src/cli/createRequest.ts +++ b/packages/cli/plugin-data-loader/src/cli/createRequest.ts @@ -112,6 +112,7 @@ export const createRequest = (routeId: string, method = 'get') => { return await parseDeferredReadableStream(res.body!); } + // some response error not from modern.js if (isOtherErrorResponse(res)) { return await handleNetworkErrorResponse(res); } diff --git a/packages/cli/plugin-data-loader/src/runtime/index.ts b/packages/cli/plugin-data-loader/src/runtime/index.ts index 10705b0b7b6b..c68fb1188188 100644 --- a/packages/cli/plugin-data-loader/src/runtime/index.ts +++ b/packages/cli/plugin-data-loader/src/runtime/index.ts @@ -130,6 +130,7 @@ export const handleRequest = async ({ }); } const cost = end(); + // add response header for client know if the response is from modern server response.headers.set('X-Modernjs-Response', 'yes'); onTiming?.(`${LOADER_REPORTER_NAME}-navigation`, cost); } catch (error) { diff --git a/packages/runtime/plugin-runtime/src/core/server/requestHandler.ts b/packages/runtime/plugin-runtime/src/core/server/requestHandler.ts index 420df0bed819..f833892b6392 100644 --- a/packages/runtime/plugin-runtime/src/core/server/requestHandler.ts +++ b/packages/runtime/plugin-runtime/src/core/server/requestHandler.ts @@ -217,7 +217,7 @@ export const createRequestHandler: CreateRequestHandler = const initialData = await runBeforeRender(context); - // Support data loader to return status code + // Support data loader to return `new Response` and set status code if ( context.routerContext?.statusCode && context.routerContext?.statusCode !== 200 diff --git a/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx b/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx index 68d88f91eb7e..59cad96aa8bd 100644 --- a/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx +++ b/packages/runtime/plugin-runtime/src/router/runtime/plugin.node.tsx @@ -125,8 +125,8 @@ export const routerPlugin = ( return interrupt(routerContext); } - // requestHandler.ts also has same logic, but handle common status code - // maybe we can put the logic in requestHandler.ts in the future + // Now `throw new Response` or `throw new Error` is same, both will be caught by errorBoundary by default + // If loaderFailureMode is 'clientRender', we will downgrade to csr, and the loader will be request in client again const errors = Object.values( (routerContext.errors || {}) as Record, ); diff --git a/tests/integration/routes/tests/index.test.ts b/tests/integration/routes/tests/index.test.ts index 77e6b0c582f0..ee5620359e68 100644 --- a/tests/integration/routes/tests/index.test.ts +++ b/tests/integration/routes/tests/index.test.ts @@ -1,5 +1,5 @@ import path from 'path'; -import { fs, ROUTE_MANIFEST_FILE, wait } from '@modern-js/utils'; +import { fs, ROUTE_MANIFEST_FILE } from '@modern-js/utils'; import { ROUTE_MANIFEST } from '@modern-js/utils/universal/constants'; import puppeteer, { type Browser } from 'puppeteer'; @@ -11,7 +11,6 @@ import { launchOptions, modernBuild, modernServe, - sleep, } from '../../../utils/modernTestUtils'; const appDir = path.resolve(__dirname, '../'); From 13656f74310791216fd50568ade3a46d926da31b Mon Sep 17 00:00:00 2001 From: kongjiacong Date: Mon, 13 Jan 2025 13:49:42 +0800 Subject: [PATCH 3/3] docs: add changeset --- .changeset/tricky-chairs-press.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/tricky-chairs-press.md diff --git a/.changeset/tricky-chairs-press.md b/.changeset/tricky-chairs-press.md new file mode 100644 index 000000000000..fa51c427e564 --- /dev/null +++ b/.changeset/tricky-chairs-press.md @@ -0,0 +1,7 @@ +--- +'@modern-js/plugin-data-loader': patch +'@modern-js/runtime': patch +--- + +feat: unify the response logic of the Loader in SSR +feat: 统一 SSR 中 Loader 的响应逻辑