-
Notifications
You must be signed in to change notification settings - Fork 218
perf: startup and cache micro-optimizations #436
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
base: main
Are you sure you want to change the base?
Changes from all commits
bd1f6c1
c0b75a5
7385c3b
df119fb
6a8a891
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,8 @@ | |
| * } | ||
| */ | ||
|
|
||
| import { Buffer } from "node:buffer"; | ||
|
|
||
| import type { CacheHandler, CacheHandlerValue, IncrementalCacheValue } from "../shims/cache.js"; | ||
| import { getRequestExecutionContext, type ExecutionContextLike } from "../shims/request-context.js"; | ||
|
|
||
|
|
@@ -66,6 +68,9 @@ const ENTRY_PREFIX = "cache:"; | |
| /** Max tag length to prevent KV key abuse. */ | ||
| const MAX_TAG_LENGTH = 256; | ||
|
|
||
| /** Matches a valid base64 string (standard alphabet with optional padding). */ | ||
| const BASE64_RE = /^[A-Za-z0-9+/]*={0,2}$/; | ||
|
|
||
|
Comment on lines
+71
to
+73
|
||
| /** | ||
| * Validate a cache tag. Returns null if invalid. | ||
| * Note: `:` is rejected because TAG_PREFIX and ENTRY_PREFIX use `:` as a | ||
|
|
@@ -435,26 +440,25 @@ function restoreArrayBuffers(value: IncrementalCacheValue): boolean { | |
| } | ||
|
|
||
| function arrayBufferToBase64(buffer: ArrayBuffer): string { | ||
| const bytes = new Uint8Array(buffer); | ||
| let binary = ""; | ||
| for (let i = 0; i < bytes.length; i++) { | ||
| binary += String.fromCharCode(bytes[i]); | ||
| } | ||
| return btoa(binary); | ||
| return Buffer.from(buffer).toString("base64"); | ||
| } | ||
|
|
||
| /** | ||
| * Decode a base64 string to an ArrayBuffer. | ||
| * Validates the input against the base64 alphabet before decoding, | ||
| * since Buffer.from(str, "base64") silently ignores invalid characters. | ||
| */ | ||
| function base64ToArrayBuffer(base64: string): ArrayBuffer { | ||
| const binary = atob(base64); | ||
| const bytes = new Uint8Array(binary.length); | ||
| for (let i = 0; i < binary.length; i++) { | ||
| bytes[i] = binary.charCodeAt(i); | ||
| if (!BASE64_RE.test(base64) || base64.length % 4 !== 0) { | ||
| throw new Error("Invalid base64 string"); | ||
| } | ||
| return bytes.buffer; | ||
| const buf = Buffer.from(base64, "base64"); | ||
| return buf.buffer.slice(buf.byteOffset, buf.byteOffset + buf.byteLength); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good use of This is a common Node.js |
||
| } | ||
|
|
||
| /** | ||
| * Safely decode base64 to ArrayBuffer. Returns null on invalid input | ||
| * instead of throwing a DOMException. | ||
| * instead of throwing. | ||
| */ | ||
| function safeBase64ToArrayBuffer(base64: string): ArrayBuffer | null { | ||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -357,15 +357,21 @@ const trustProxy = process.env.VINEXT_TRUST_PROXY === "1" || trustedHosts.size > | |
|
|
||
| /** | ||
| * Convert a Node.js IncomingMessage to a Web Request object. | ||
| * | ||
| * When `urlOverride` is provided, it is used as the path + query string | ||
| * instead of `req.url`. This avoids redundant path normalization when the | ||
| * caller has already decoded and normalized the pathname (e.g. the App | ||
| * Router prod server normalizes before static-asset lookup, and can pass | ||
| * the result here so the downstream RSC handler doesn't re-normalize). | ||
| */ | ||
| function nodeToWebRequest(req: IncomingMessage): Request { | ||
| function nodeToWebRequest(req: IncomingMessage, urlOverride?: string): Request { | ||
| const rawProto = trustProxy | ||
| ? (req.headers["x-forwarded-proto"] as string)?.split(",")[0]?.trim() | ||
| : undefined; | ||
| const proto = rawProto === "https" || rawProto === "http" ? rawProto : "http"; | ||
| const host = resolveHost(req, "localhost"); | ||
| const origin = `${proto}://${host}`; | ||
| const url = new URL(req.url ?? "/", origin); | ||
| const url = new URL(urlOverride ?? req.url ?? "/", origin); | ||
|
|
||
| const headers = new Headers(); | ||
| for (const [key, value] of Object.entries(req.headers)) { | ||
|
|
@@ -601,9 +607,9 @@ async function startAppRouterServer(options: AppRouterServerOptions) { | |
| const rscHandler = resolveAppRouterHandler(rscModule.default); | ||
|
|
||
| const server = createServer(async (req, res) => { | ||
| const url = req.url ?? "/"; | ||
| const rawUrl = req.url ?? "/"; | ||
| // Normalize backslashes (browsers treat /\ as //), then decode and normalize path. | ||
| const rawPathname = url.split("?")[0].replaceAll("\\", "/"); | ||
| const rawPathname = rawUrl.split("?")[0].replaceAll("\\", "/"); | ||
| let pathname: string; | ||
| try { | ||
| pathname = normalizePath(decodeURIComponent(rawPathname)); | ||
|
|
@@ -630,7 +636,7 @@ async function startAppRouterServer(options: AppRouterServerOptions) { | |
| // Image optimization passthrough (Node.js prod server has no Images binding; | ||
| // serves the original file with cache headers and security headers) | ||
| if (pathname === IMAGE_OPTIMIZATION_PATH) { | ||
| const parsedUrl = new URL(url, "http://localhost"); | ||
| const parsedUrl = new URL(rawUrl, "http://localhost"); | ||
| const defaultAllowedWidths = [...DEFAULT_DEVICE_SIZES, ...DEFAULT_IMAGE_SIZES]; | ||
| const params = parseImageParams(parsedUrl, defaultAllowedWidths); | ||
| if (!params) { | ||
|
|
@@ -663,8 +669,14 @@ async function startAppRouterServer(options: AppRouterServerOptions) { | |
| } | ||
|
|
||
| try { | ||
| // Build the normalized URL (pathname + original query string) so the | ||
| // RSC handler receives an already-canonical path and doesn't need to | ||
| // re-normalize. This deduplicates the normalizePath work done above. | ||
| const qs = rawUrl.includes("?") ? rawUrl.slice(rawUrl.indexOf("?")) : ""; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edge case: One thing to double-check: if |
||
| const normalizedUrl = pathname + qs; | ||
|
Comment on lines
+675
to
+676
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: This query-string extraction works correctly, but note that if The approach is sound and avoids the cost of |
||
|
|
||
| // Convert Node.js request to Web Request and call the RSC handler | ||
| const request = nodeToWebRequest(req); | ||
| const request = nodeToWebRequest(req, normalizedUrl); | ||
| const response = await rscHandler(request); | ||
|
|
||
| // Stream the Web Response back to the Node.js response | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex matches the empty string (
"") since*and{0,2}both allow zero occurrences.Buffer.from("", "base64")produces an empty Buffer, which would decode to a zero-length ArrayBuffer — probably not what you want for a cache entry, and the oldatob("")also returned""so this is not a regression, but it's worth noting.Also, as Copilot flagged: the regex validates the alphabet but not structural validity. For example,
"A"(length 1, which is% 4 === 1) is structurally invalid base64 — but you already handle that with thebase64.length % 4 !== 0check on line 396, so this is fine. The regex + length check together are sufficient for correctness.No change needed — just confirming the validation is adequate as-is.