Feature/native server#281
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| const overrides: Record<PropertyKey, unknown> = { | ||
| body, | ||
| cookies, | ||
| headers, | ||
| params, | ||
| queries, | ||
| rawBody, | ||
| rawHeaders | ||
| }; | ||
| return new Proxy(request, { | ||
| get(target, key) { | ||
| if (key in overrides) return overrides[key]; | ||
| const value = Reflect.get(target, key, target); | ||
| return typeof value === 'function' ? value.bind(target) : value; | ||
| }, | ||
| has(target, key) { | ||
| return key in overrides || Reflect.has(target, key); | ||
| } | ||
| }) as unknown as MockServerRequest; | ||
| }; |
There was a problem hiding this comment.
чтобы не перезаписывать изначальный request (у него немутируемые свойства), а просто перехватывать обращения ко свойствам и отдавать нужное поле
|
@greptile can you do review now? |
Greptile SummaryThis PR introduces
|
| Filename | Overview |
|---|---|
| packages/server/src/server/createNativeMockServer/createNativeMockServer.ts | Core native HTTP server entrypoint; handler loop with NextError-based routing is well-structured and error-safe. |
| packages/server/src/server/createNativeMockServer/helpers/http/sendFetchResponse/sendFetchResponse.ts | Bridges Fetch Response to Node ServerResponse; setHeaders will corrupt multiple Set-Cookie headers (values comma-joined by Fetch Headers API). Known TODO in file. |
| packages/server/src/server/createNativeMockServer/helpers/http/toFetchRequest/toFetchRequest.ts | Converts IncomingMessage to Fetch Request; method/body gating and duplex handling are correct. |
| packages/server/src/server/createNativeMockServer/helpers/http/toMockServerRequest/toMockServerRequest.ts | Body parsing and Proxy-based MockServerRequest construction look correct; minor inconsistency with missing await on some async branches. |
| packages/server/src/server/createNativeMockServer/helpers/routes/createRestRoute.ts | Core REST route logic; P1 bug: using Headers.set() in the final header merge loop silently drops all but the last set-cookie when multiple cookies are set. |
| packages/server/src/server/createNativeMockServer/helpers/interceptors/callResponseInterceptors.ts | Chains response interceptors correctly; cookie serialization uses the new cookie package. |
| packages/server/src/server/createNativeMockServer/types/interceptors.ts | Defines native interceptor types but imports CookieOptions from express, creating an Express coupling in the native server module. |
| packages/server/src/server/createNativeMockServer/types/rest.ts | REST type definitions; same Express CookieOptions coupling as interceptors.ts. |
| packages/server/src/server/startMockServer/startMockServer.ts | Swapped from Express createMockServer to native createNativeMockServer; destroyerMiddleware is compatible with node:http.Server so no breakage there. |
| packages/server/tsconfig.production.json | Removed lib: [dom, esnext]; intentional shift to rely on @types/node for Fetch/URL globals rather than the DOM lib typings. |
| packages/server/src/server/createNativeMockServer/helpers/routes/prepareRestRoute.ts | Builds REST request artifacts from server components and wires interceptors at all four levels; logic mirrors existing Express-based implementation. |
Comments Outside Diff (3)
-
packages/server/src/server/createNativeMockServer/helpers/routes/createRestRoute.ts, line 591-594 (link)set-cookieheaders silently dropped when multiple cookies are setHeaders.entries()yields eachset-cookieentry as a separate item. CallingfinalHeaders.set(name, value)inside the loop overwrites the header on every iteration, so only the last cookie survives. This means any response that sets more than one cookie (e.g. auth token + session) will silently discard all but the final one. -
packages/server/src/server/createNativeMockServer/helpers/http/sendFetchResponse/sendFetchResponse.ts, line 92-101 (link)set-cookieheaders flattened/lost viasetHeadersserverResponse.setHeaders(fetchResponse.headers)relies on the FetchHeadersAPI, which comma-joins multipleset-cookievalues into a single string. Cookie values may themselves contain commas, producing a malformed header. The file's own TODO flags this — it needs explicitset-cookiehandling (viafetchResponse.headers.getSetCookie()) before the bulksetHeaderscall. -
packages/server/src/server/createNativeMockServer/helpers/http/toMockServerRequest/toMockServerRequest.ts, line 163-202 (link)Inconsistent
awaitinparseRawBodyresponse.json()is awaited (return await response.json()), butresponse.arrayBuffer()(lines 169, 202) andresponse.formData()(line 186) are returned withoutawait. All branches return Promises and the caller awaits the result, so this works, but the inconsistency is confusing and may mask future bugs if the return type changes.
Reviews (1): Last reviewed commit: "feature/native-server 💎 fix bugs" | Re-trigger Greptile
| @@ -0,0 +1,38 @@ | |||
| import type { CookieOptions } from 'express'; | |||
There was a problem hiding this comment.
Express type dependency in native server
CookieOptions is imported from express, coupling this "native" (Express-free) server to the Express package for a type-only reason. The same import appears in types/rest.ts. Consider defining a local CookieOptions interface or importing from the standalone cookie package (already added as a dependency in this PR) to avoid the Express coupling.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const isOptionSettings = !('configs' in option); | ||
| const mockServerSettings = isOptionSettings ? option : {}; | ||
| const mockServerComponents = ( | ||
| isOptionSettings ? mockServerConfig.slice(1) : mockServerConfig | ||
| ) as MockServerComponent[]; |
There was a problem hiding this comment.
почему переделали тут кодец с оригинального сервера
| ) as MockServerComponent[]; | ||
|
|
||
| const restRoute = prepareRestRoute(mockServerSettings, mockServerComponents); | ||
| const handle404 = () => new Response(null, { status: 404 }); |
| const restRoute = createRestRoute({ restRequestArtifacts }); | ||
|
|
||
| return restRoute; |
There was a problem hiding this comment.
return createRestRoute({ restRequestArtifacts });
| const dataResponse = | ||
| resolvedData instanceof Response | ||
| ? copyResponseWith(resolvedData, { | ||
| headers: responseState.headers, | ||
| status: responseState.statusCode ?? matchedRouteConfig.config.settings?.status | ||
| }) | ||
| : Response.json(resolvedData, { | ||
| headers: responseState.headers, | ||
| status: responseState.statusCode ?? matchedRouteConfig.config.settings?.status | ||
| }); |
There was a problem hiding this comment.
возможно стоит оставить все таки 1 формат
No description provided.