typescript migration: all source files, not including tests and build scripts#153
typescript migration: all source files, not including tests and build scripts#153
Conversation
WalkthroughAdds TypeScript support and tooling: new TypeScript source modules (validation, path resolution, file reader, schemas), build/type-generation scripts, package.json exports/types, tsconfig, tests updated to use dist, and documentation/examples for migration and TypeScript usage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
jstjoe
changed the title
|
📝 Documentation updates detected! Updated existing suggestion: Document testing requirements and TypeScript codebase for doc-detective-common |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@docs/typescript-migration.md`:
- Around line 569-571: Replace the three bullet lines that contain bare URLs
(the lines starting with "**Issues**:", "**Discussions**:", and
"**Documentation**:") so each URL is wrapped as a markdown link (e.g.,
[text](URL)); update the Issues and Discussions bullets to use link text like
"Report bugs or request features" and "Ask questions" and change the
Documentation bullet to link to the README using bracketed text instead of raw
URL to satisfy markdownlint MD034.
In `@package.json`:
- Around line 37-43: Update the devDependencies: bump "@types/node" to the
current stable (25.0.6) and "typescript" to 5.9 in package.json, run your
package manager to refresh lockfile (npm/yarn/pnpm) and ensure the project
compiles and tests pass (tsc and test scripts); also verify the
"json-schema-to-typescript" entry is intentional (confirm registry name/version
or replace with the correct scoped/fork package) and update its version if
needed before committing the lockfile changes.
In `@plans/plan-typescriptMigration.prompt.md`:
- Line 17: The wording in Step 6 is inconsistent with the real pipeline order;
update the description in plans/plan-typescriptMigration.prompt.md to reflect
that schema dereference runs before type generation and compilation by changing
the step text to indicate the actual sequence: dereferenceSchemas →
generate:types → compile, and remove or reword the phrase that currently says to
run tsc prior to dereference; reference the existing step mentioning
dereferenceSchemas.js and the tasks generate:types and compile so the step
aligns with the pipeline order shown in the PR.
In `@scripts/generateTypes.js`:
- Around line 16-37: The current loop over v3Schemas logs compilation/write
errors but swallows them; update the try/catch around compile(...) and
fs.writeFile(...) to record failures (e.g., push file names or set a boolean
like hadFailures) inside the catch instead of only logging, and after the for
(const file of v3Schemas) loop check that failure flag/array and throw a new
Error (including a summary or count and the failed filenames) so the outer error
handler at lines 42-44 can exit with code 1; reference the compile(...) call,
the catch(error) block, and the write to outputPath to locate where to record
failures and where to throw.
In `@src/files.ts`:
- Around line 21-30: The readFile function currently throws on invalid input and
will break when called with undefined; change the signature to provide a default
for the options parameter (e.g., ReadFileOptions = {}) so destructuring doesn't
throw, remove the explicit throws for fileURLOrPath validation, and instead if
fileURLOrPath is missing, not a string, or empty call a logger.warn (or
console.warn) with a clear message and return null; also wrap the file reading
logic in a try/catch so any runtime error is logged as a warning and the
function returns null (refer to readFile, ReadFileOptions, and fileURLOrPath to
locate the code).
In `@src/validate.ts`:
- Around line 507-522: The openApi_v3 branch constructs transformedObject then
calls validate(...) but returns transformedObject rather than the validation
result that may include defaults; update the branch so that after calling
validate({ schemaKey: "openApi_v3", object: transformedObject }) you check
result.valid and then return result.object (not transformedObject), preserving
the existing error throw path; adjust references to
transformedObject/intermediaryObject/requestHeaders as needed so you pass the
same transformed input to validate but return result.object when valid.
🧹 Nitpick comments (2)
scripts/createEsmWrapper.js (1)
4-15: Create the dist directory before writing the wrapper.If the script is run standalone or after a failed compile,
writeFilewill throw becausedist/might not exist. Consider ensuring the directory exists.♻️ Suggested change
async function createEsmWrapper() { const distDir = path.join(__dirname, "..", "dist"); + await fs.mkdir(distDir, { recursive: true });src/resolvePaths.ts (1)
153-156: Potential null safety issue withObject.keys(object).If
objectisnull, the conditionobject === nullshort-circuits correctly. However, this check happens afterpathPropertiesassignment which already uses theobjectparameter. Consider moving this check earlier in the function.♻️ Suggested improvement: Move null check earlier
export async function resolvePaths({ config, object, filePath, nested = false, objectType, }: ResolvePathsOptions): Promise<Record<string, any>> { + // If the object is null or empty, return it as is + if (object === null || Object.keys(object).length === 0) { + return object; + } + // Config properties that contain paths const configPaths = [And remove lines 153-156.
| "@types/node": "^22.10.5", | ||
| "c8": "^10.1.3", | ||
| "chai": "^6.2.2", | ||
| "json-schema-to-typescript": "^15.0.4", | ||
| "mocha": "^11.7.5", | ||
| "sinon": "^21.0.1" | ||
| "sinon": "^21.0.1", | ||
| "typescript": "^5.7.3" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version @types/node npm package 2025 2026
💡 Result:
As of January 17, 2026 the latest published @types/node version appears to be 25.0.6. [1][2]
Sources:
- NuGet/ProGet listing showing latest 25.0.6. [1]
- ReleaseAlert / npm release history (shows 25.x releases through Dec 2025). [2]
🌐 Web query:
latest version typescript npm package 2025 2026
💡 Result:
As of January 17, 2026 the latest stable TypeScript release is TypeScript 5.9 (released December 2, 2025). Install with: npm install -D typescript. [1][2]
Sources:
- TypeScript blog — Announcing TypeScript 5.9 (Dec 2, 2025). [1]
- npm package page / published versions (TypeScript releases and dev tags). [2]
🌐 Web query:
latest version mocha npm package 2025 2026
💡 Result:
As of January 17, 2026:
- Latest stable release: mocha 11.7.5. [1][2]
- Latest prerelease (beta): 12.0.0-beta-4 (published January 4, 2026). [1]
Sources: Yarn package page for mocha (shows tags, versions and dates). [1] Snyk package page (confirms latest stable 11.7.5). [2]
References
[1] yarnpkg.com — mocha package details (versions/tags).
[2] security.snyk.io — mocha package summary.
🌐 Web query:
latest version chai npm package 2025 2026
💡 Result:
The latest published version of the npm package "chai" is 6.2.2. [1][2]
Sources:
- Snyk package page showing latest version 6.2.2. [1]
- Socket / package registry mirror listing latest version 6.2.2. [2]
🌐 Web query:
latest version sinon npm package 2025 2026
💡 Result:
As of Jan 17, 2026 the latest released sinon version appears to be 21.0.1 (21.0.0 is listed on npm’s package page). [1][2]
- Snyk reports latest: 21.0.1. [1]
- npm package listing shows 21.0.0 (published Jun 13, 2025). [2]
If you want, I can fetch the exact npm publish timestamps or show the npm install command for a specific version.
🌐 Web query:
latest version c8 npm package 2025 2026
💡 Result:
The latest published npm version of the c8 package is 10.1.3 (as shown on npm). [1][2]
Sources:
[1] npm package page for c8 — version 10.1.3.
[2] Snyk package summary referencing latest version 10.1.3.
🌐 Web query:
latest version json-schema-to-typescript npm package 2025 2026
💡 Result:
As of Jan 17, 2026:
- The json-schema-to-typescript package on npm is at version 8.2.0 (npm listing). [1]
- A maintained fork named json-schema-to-typescript-lite has a more recent release (v15.0.0). [2]
Sources:
[1] npm package page for json-schema-to-typescript.
[2] npm package page for json-schema-to-typescript-lite.
Update @types/node and TypeScript to current stable versions.
@types/nodeis at ^22.10.5 but latest stable is 25.0.6 (3 major versions behind)typescriptis at ^5.7.3 but latest stable is 5.9 (2 minor versions behind)- Other dependencies (c8, chai, mocha, sinon) are current
- Note:
json-schema-to-typescriptat ^15.0.4 appears inconsistent with npm registry data; verify this is intentional or if a fork/scoped package is intended
🤖 Prompt for AI Agents
In `@package.json` around lines 37 - 43, Update the devDependencies: bump
"@types/node" to the current stable (25.0.6) and "typescript" to 5.9 in
package.json, run your package manager to refresh lockfile (npm/yarn/pnpm) and
ensure the project compiles and tests pass (tsc and test scripts); also verify
the "json-schema-to-typescript" entry is intentional (confirm registry
name/version or replace with the correct scoped/fork package) and update its
version if needed before committing the lockfile changes.
| export async function readFile({ fileURLOrPath }: ReadFileOptions): Promise<unknown | string | null> { | ||
| if (!fileURLOrPath) { | ||
| throw new Error("fileURLOrPath is required"); | ||
| } | ||
| if (typeof fileURLOrPath !== "string") { | ||
| throw new Error("fileURLOrPath must be a string"); | ||
| } | ||
| if (fileURLOrPath.trim() === "") { | ||
| throw new Error("fileURLOrPath cannot be an empty string"); | ||
| } |
There was a problem hiding this comment.
Avoid throwing for invalid input; return null with warnings.
Current validation throws (and destructuring will throw if called with undefined), which breaks the documented graceful-degradation behavior. Recommend defaulting the options param and returning null with warnings instead. Based on learnings, readFile should return null on errors and log warnings.
🐛 Suggested fix
-export async function readFile({ fileURLOrPath }: ReadFileOptions): Promise<unknown | string | null> {
- if (!fileURLOrPath) {
- throw new Error("fileURLOrPath is required");
- }
- if (typeof fileURLOrPath !== "string") {
- throw new Error("fileURLOrPath must be a string");
- }
- if (fileURLOrPath.trim() === "") {
- throw new Error("fileURLOrPath cannot be an empty string");
- }
+export async function readFile(
+ { fileURLOrPath }: ReadFileOptions = { fileURLOrPath: "" }
+): Promise<unknown | string | null> {
+ if (typeof fileURLOrPath !== "string" || fileURLOrPath.trim() === "") {
+ console.warn("fileURLOrPath must be a non-empty string");
+ return null;
+ }🤖 Prompt for AI Agents
In `@src/files.ts` around lines 21 - 30, The readFile function currently throws on
invalid input and will break when called with undefined; change the signature to
provide a default for the options parameter (e.g., ReadFileOptions = {}) so
destructuring doesn't throw, remove the explicit throws for fileURLOrPath
validation, and instead if fileURLOrPath is missing, not a string, or empty call
a logger.warn (or console.warn) with a clear message and return null; also wrap
the file reading logic in a try/catch so any runtime error is logged as a
warning and the function returns null (refer to readFile, ReadFileOptions, and
fileURLOrPath to locate the code).
|
@hawkeyexl I'd love to get your eyes on this. Based on the CodeRabbit comments I do see we'll need to update the CI steps but I think most of the other points are feedback on the existing implementation? I don't know this code well enough to say. |
- Fix coverage config to measure dist/ instead of src/ - Update CI workflow to build before coverage check - Add integration tests for CJS and ESM imports - Fix bare URLs in docs (MD034) - Add error tracking to generateTypes.js - Ensure dist directory exists in createEsmWrapper.js - Fix openApi_v3 transformation to return result.object - Add test for invalid objectType error - Fix c8 ignore comment coverage
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/createEsmWrapper.js`:
- Around line 1-24: The ESM wrapper generated by createEsmWrapper() (esmContent)
only re-exports runtime values and omits TypeScript types; update the export
list inside esmContent to include the type names (SchemaKey, Schema,
ValidateOptions, ValidateResult, TransformOptions, ResolvePathsOptions,
ReadFileOptions) alongside schemas, validate, transformToSchemaKey,
resolvePaths, readFile so TypeScript consumers get both runtime and type
exports; modify the string built in createEsmWrapper() (variable esmContent) to
include those type identifiers in the exported destructuring from cjsModule.
In `@test/integration.test.js`:
- Around line 6-10: The test file is wrapped in an async IIFE which prevents
Mocha from discovering describe/it suites; remove the IIFE and make imports
synchronous: replace the dynamic await import("chai") with a top-level const {
expect } = require("chai") and keep the existing const path = require("path")
and const { execSync } = require("child_process") at the top level so
describe/it are registered synchronously (check the describe/it blocks
referenced in this file to ensure they are no longer inside any async wrapper).
🧹 Nitpick comments (1)
src/validate.ts (1)
283-290: PotentialNaNfrom dividing undefinedmaxVariation.If
object.maxVariationisundefined, the divisionobject.maxVariation / 100producesNaN. This occurs in multiple transformations (lines 285, 313, 335, 358).Consider using nullish coalescing or conditional assignment:
Suggested fix
- maxVariation: object.maxVariation / 100, + maxVariation: object.maxVariation != null ? object.maxVariation / 100 : undefined,The validation step afterward may handle this, but passing explicit
undefinedis cleaner thanNaN.
| const fs = require("fs").promises; | ||
| const path = require("path"); | ||
|
|
||
| async function createEsmWrapper() { | ||
| const distDir = path.join(__dirname, "..", "dist"); | ||
|
|
||
| // Ensure dist directory exists | ||
| await fs.mkdir(distDir, { recursive: true }); | ||
|
|
||
| // Create ESM wrapper that re-exports from CJS | ||
| const esmContent = `// ESM wrapper for CommonJS output | ||
| import cjsModule from './index.js'; | ||
| export const { schemas, validate, transformToSchemaKey, resolvePaths, readFile } = cjsModule; | ||
| export default cjsModule; | ||
| `; | ||
|
|
||
| await fs.writeFile(path.join(distDir, "index.mjs"), esmContent); | ||
| console.log("Created ESM wrapper at dist/index.mjs"); | ||
| } | ||
|
|
||
| createEsmWrapper().catch((error) => { | ||
| console.error("Failed to create ESM wrapper:", error); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine src/index.ts
echo "=== Checking src/index.ts exports ==="
if [ -f "src/index.ts" ]; then
cat -n src/index.ts
else
echo "src/index.ts not found, searching for it..."
find . -name "index.ts" -type f | head -10
fiRepository: doc-detective/common
Length of output: 418
Add missing type exports to the ESM wrapper.
The wrapper currently only re-exports the function and object values but omits the TypeScript types. Update the ESM wrapper to include the type exports:
export const { schemas, validate, transformToSchemaKey, resolvePaths, readFile, SchemaKey, Schema, ValidateOptions, ValidateResult, TransformOptions, ResolvePathsOptions, ReadFileOptions } = cjsModule;This ensures TypeScript consumers have full access to both runtime exports and type definitions when importing from the ESM module.
🤖 Prompt for AI Agents
In `@scripts/createEsmWrapper.js` around lines 1 - 24, The ESM wrapper generated
by createEsmWrapper() (esmContent) only re-exports runtime values and omits
TypeScript types; update the export list inside esmContent to include the type
names (SchemaKey, Schema, ValidateOptions, ValidateResult, TransformOptions,
ResolvePathsOptions, ReadFileOptions) alongside schemas, validate,
transformToSchemaKey, resolvePaths, readFile so TypeScript consumers get both
runtime and type exports; modify the string built in createEsmWrapper()
(variable esmContent) to include those type identifiers in the exported
destructuring from cjsModule.
| (async () => { | ||
| const { expect } = await import("chai"); | ||
| const path = require("path"); | ||
| const { execSync } = require("child_process"); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/integration.test.js | head -50Repository: doc-detective/common
Length of output: 2306
🏁 Script executed:
wc -l test/integration.test.jsRepository: doc-detective/common
Length of output: 92
🏁 Script executed:
git ls-files test/Repository: doc-detective/common
Length of output: 175
🏁 Script executed:
tail -15 test/integration.test.js | cat -nRepository: doc-detective/common
Length of output: 768
🏁 Script executed:
cd test/integration.test.js 2>/dev/null || true && npx mocha test/integration.test.js --reporter spec 2>&1 | head -100Repository: doc-detective/common
Length of output: 170
Async IIFE prevents Mocha from discovering tests—fix by using synchronous imports and top-level definitions.
The async IIFE wrapping the describe calls is causing Mocha to discover zero tests (confirmed: test run shows "0 passing"). Mocha loads test files synchronously and doesn't await the IIFE, so test suites are never registered before Mocha's discovery phase completes. Replace await import("chai") with require("chai") and remove the IIFE wrapper to allow synchronous test registration.
🐛 Proposed fix
-(async () => {
- const { expect } = await import("chai");
+const { expect } = require("chai");
const path = require("path");
const { execSync } = require("child_process");
describe("Module Integration Tests", function () {
// ... test definitions
-})();Also applies to: 214-215
🤖 Prompt for AI Agents
In `@test/integration.test.js` around lines 6 - 10, The test file is wrapped in an
async IIFE which prevents Mocha from discovering describe/it suites; remove the
IIFE and make imports synchronous: replace the dynamic await import("chai") with
a top-level const { expect } = require("chai") and keep the existing const path
= require("path") and const { execSync } = require("child_process") at the top
level so describe/it are registered synchronously (check the describe/it blocks
referenced in this file to ensure they are no longer inside any async wrapper).
hawkeyexl
changed the title
|
Spoke with @jstjoe offline. Removed |
|
📝 Documentation updates detected! Updated existing suggestion: Document testing requirements and TypeScript codebase for doc-detective-common |
|
+1 sorry for the delay! |
2 participants
Migrate to TypeScript
Converts the entire codebase (except tests) from vanilla JavaScript to TypeScript while maintaining 100% backward compatibility and test coverage.
🎯 Goals Achieved
📦 Changed Files
Source Files Converted (src/):
index.ts- Main barrel export with type re-exportsfiles.ts- File reading utility with proper type unionsresolvePaths.ts- Path resolution with discriminated unionsschemas/index.ts- Typed schema mapBuild Infrastructure:
Test Updates:
🔧 Technical Details
New Dependencies:
typescript^5.7.3@types/node^22.10.5json-schema-to-typescript^15.0.4Build Pipeline:
Output Structure (dist/):
require())import)dist/**/*.d.ts- TypeScript declaration filesdist/**/*.d.ts.map- Declaration source mapsdist/**/*.js.map- JavaScript source mapsPackage Exports:
{ "main": "dist/index.js", "types": "dist/index.d.ts", "exports": { ".": { "require": "./dist/index.js", "import": "./dist/index.mjs", "types": "./dist/index.d.ts" } } }🧪 Testing
All 413 tests pass without modifications:
Test Coverage:
🔒 Backward Compatibility
No Breaking Changes:
require('doc-detective-common')New Capabilities:
import { validate } from 'doc-detective-common'📝 Type Safety Approach
anytypes used strategically in validate.ts for dynamic transformation logic (necessary due to runtime validation patterns via AJV)Generated Types:
StepV3,ConfigV3,SpecV3,TestV3,ContextV3, etc.✅ Verification Checklist
npx tsc --noEmit --strict🚀 Usage for Downstream Consumers
JavaScript (unchanged):
TypeScript (new):
📋 Notes
.jsfiles kept in src for reference during transitionSummary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.