Conversation
WalkthroughThe codebase migrates well-known headers from a JSON file approach to a TypeScript module approach. The generation tool now produces a TypeScript file with a default export, imports are updated accordingly, and the generation script integrates prettier formatting. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes follow a consistent, homogeneous pattern of migrating JSON to TypeScript. Individual modifications are straightforward—simple import/export syntax updates and a moderate build tool enhancement. Logic density is low, and the repetitive nature of the pattern reduces review complexity. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/headers.ts (1)
241-253: CSV parsing is brittle (CRLF, whitespace) and can drop rows — normalize and trim; also dedupe and sort.IANA CSV may use CRLF and trailing whitespace. Current logic can misclassify
category(e.g.,permanent\r) and filter out everything. Also, no dedupe/sort leads to unstable output.Apply:
-const lines = csv.split("\n").slice(1, -1); +const lines = csv.split(/\r?\n/).slice(1).filter(Boolean); const headers = lines - .map((line) => { - const [name, category] = line.split(",").slice(0, 2); - return { name, category }; - }) + .map((line) => { + const [nameRaw, categoryRaw] = line.split(",").slice(0, 2); + const name = nameRaw?.trim(); + const category = categoryRaw?.trim(); + return { name, category }; + }) .filter( ({ name, category }) => /^[\w-]+$/.test(name) && categories.includes(category), ) - .map(({ name }) => name.toLowerCase()) - .filter((name) => !(name in responseOnlyHeaders)); + .map(({ name }) => name.toLowerCase()) + .filter((name) => !(name in responseOnlyHeaders)); + +// stabilize and dedupe +const headersSet = new Set(headers); +const stableHeaders = Array.from(headersSet).sort();And below replace subsequent uses of
headerswithstableHeaders.
🧹 Nitpick comments (3)
tools/headers.ts (1)
256-258: Emit a banner and readonly typing in the generated TS for clarity and safety.Mark the file as generated and hint immutability. Prettier will keep formatting stable.
-const json = JSON.stringify(headers, undefined, 2); -const tsCode = await format(`export default ${json};`, { filepath: dest }); +const json = JSON.stringify(stableHeaders ?? headers, undefined, 2); +const tsCode = await format( + `// This file is auto-generated by tools/headers.ts. Do not edit manually. +export default ${json} as string[]; +`, + { filepath: dest }, +); await writeFile(dest, tsCode, "utf-8");Optional next step: generate a Set for O(1) membership checks if lookups are hot.
express-zod-api/src/well-known-headers.ts (1)
1-205: LGTM, matches the new import shape.Optional: add a header comment indicating the file is generated and consider freezing or exporting a Set for faster membership checks.
Example:
// Auto-generated by tools/headers.ts. Do not edit. export default /* readonly */ [ // ... ] as string[];express-zod-api/src/documentation-helpers.ts (1)
48-48: TS extension import confirmed; optional refactoring suggestion is valid.Verification passed:
allowImportingTsExtensionsis enabled in the roottsconfig.json, so the.tsextension import is supported.The optional refactoring suggestion is sound:
defaultIsHeader()at line 273-274 useswellKnownHeaders.includes()on a large array (~260+ entries). Switching to aSetwould improve lookup performance from O(n) to O(1), which is worth considering since this function can be called multiple times per request during header parameter processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
express-zod-api/src/documentation-helpers.ts(1 hunks)express-zod-api/src/well-known-headers.ts(2 hunks)tools/headers.ts(3 hunks)
🔇 Additional comments (2)
tools/headers.ts (2)
3-3: No issue — Prettier v3.6.2 is already in use.The repository uses Prettier 3.6.2 (devDependencies in package.json), which fully supports named exports. The import syntax is valid and compatible.
212-212: All prerequisites for TS extension imports are already satisfied.The original review comment's concerns are already addressed:
allowImportingTsExtensionsis enabled: The express-zod-api/tsconfig.json extends the root tsconfig.json, which hasallowImportingTsExtensions: true. This setting applies to the subdirectory.Bundler is in place: tsdown (modern successor to tsup) is configured as the build tool and performs module specifier rewriting during the build.
Only compiled artifacts are published: The package.json exports only
dist/index.jsanddist/index.d.ts—TypeScript source files are not included in the distribution.Likely an incorrect or invalid review comment.
1 participant
Need this for #3011
For some weird reason it's no longer working with latest rolldown.
Todo: align the generator tool
Summary by CodeRabbit