Conversation
…d identifier validation The changes in this commit introduce a new `CompilerConfig` singleton that manages the TypeScript compiler options and script target configuration for the codegen system. The key changes are: 1. Automatic script target detection: The system now automatically determines the appropriate TypeScript script target from the `ts-morph` Project's compiler options. 2. Identifier validation: Property names in generated TypeBox objects are validated using TypeScript's built-in utilities (`ts.isIdentifierStart()` and `ts.isIdentifierPart()`). This ensures compatibility with the detected script target. 3. Configuration management: The `CompilerConfig` singleton provides a centralized way to manage the script target configuration, allowing it to be overridden per-project as needed. 4. Default behavior: When no explicit target is specified, the system falls back to `ts.ScriptTarget.Latest`, providing maximum compatibility with modern JavaScript features. 5. Integration points: The configuration system is integrated with various components of the codegen system, including the Input Handler, Code Generation, Identifier Utils, and Object Handlers. These changes improve the overall robustness and flexibility of the codegen system, ensuring that the generated code is compatible with the target JavaScript environment.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a CompilerConfig singleton with environment- and project-based script-target detection, an isValidIdentifier utility, target-aware object property-name extraction, wires compiler initialization into entry points, refactors several TypeBox operator handlers (moves/removes/adds), and adds documentation and tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI / Caller
participant Project as ts-morph Project
participant Config as CompilerConfig (singleton)
participant Gen as Code Generator
participant Id as Identifier Utils
CLI->>Project: new Project({ compilerOptions })
CLI->>Config: initializeCompilerConfig(Project)
Config->>Project: read compilerOptions.target
Config-->>CLI: scriptTarget stored
CLI->>Gen: generate(sourceFile)
Gen->>Id: isValidIdentifier(name)
Id->>Config: getScriptTarget()
Config-->>Id: scriptTarget
Id-->>Gen: boolean (identifier?)
Gen-->>CLI: emit PropertyAssignment (Identifier or StringLiteral)
sequenceDiagram
autonumber
participant Handler as ObjectLikeBaseHandler
participant Prop as PropertySignature
participant AST as TS AST
participant Id as isValidIdentifier
Handler->>Prop: getNameNode()
Prop-->>AST: return nameNode (Identifier/StringLiteral/NumericLiteral/Other)
alt Identifier node
Handler-->>Handler: use Identifier
else NumericLiteral
Handler-->>Handler: convert to Identifier node
else StringLiteral
Handler->>Id: isValidIdentifier(literalValue)
alt true
Handler-->>Handler: use Identifier
else false
Handler-->>Handler: use StringLiteral
end
else fallback
Handler-->>Handler: use prop.getName() text
end
Handler-->>Handler: build PropertyAssignment with chosen name node
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
tests/handlers/typebox/interface-generics-runtime-binding.test.ts (2)
37-37: Same note as above about instantiation expressions.Also applies to: 48-61
64-64: Same note as above about instantiation expressions.Also applies to: 79-96
🧹 Nitpick comments (19)
tests/traverse/dependency-traversal.integration.test.ts (1)
118-118: Prefer explicit filename for consistency.Other tests in this suite use 'main.ts'. Using the default 'test.ts' here is fine functionally but slightly inconsistent and can obscure relative import resolution when debugging.
Apply this diff for consistency:
- const mainFile = createSourceFile(project, 'import { NonExistent } from "./non-existent";') + const mainFile = createSourceFile(project, 'import { NonExistent } from "./non-existent";', 'main.ts')src/utils/compiler-config.ts (3)
54-63: Simplify determineScriptTarget and fix misleading commentUse a one-liner fallback; the current comment suggests mappings that aren’t implemented.
- private determineScriptTarget(compilerOptions: ts.CompilerOptions): ts.ScriptTarget { - // If target is explicitly set in compiler options, use it - if (compilerOptions.target !== undefined) { - return compilerOptions.target - } - - // Default fallback based on common configurations - // ESNext maps to Latest, ES2022+ maps to ES2022, etc. - return ts.ScriptTarget.Latest - } + private determineScriptTarget(compilerOptions: ts.CompilerOptions): ts.ScriptTarget { + return compilerOptions.target ?? ts.ScriptTarget.Latest + }
7-9: Global state can be clobbered across multiple ProjectsA single stored scriptTarget means later initializations overwrite earlier ones. Given multiple Projects may exist, consider scoping config per Project (Map<Project, ScriptTarget>) or eliminating global state in favor of explicit parameters.
68-70: Add static reset for clean test/process isolationInstance-level reset is fine, but a static “resetInstance” helps ensure no cross-test leakage.
reset(): void { this.scriptTarget = ts.ScriptTarget.Latest } + + static resetInstance(): void { + CompilerConfig.instance = null + }src/index.ts (1)
12-18: Reuse the input Project to avoid extra Program and ensure consistent settingsPassing the input SourceFile’s Project removes duplication and eliminates accidental config drift.
-const createOutputFile = (hasGenericInterfaces: boolean) => { - const project = new Project() - - const newSourceFile = project.createSourceFile('output.ts', '', { +const createOutputFile = (hasGenericInterfaces: boolean, project: Project) => { + const newSourceFile = project.createSourceFile('output.ts', '', { overwrite: true, }) @@ - const newSourceFile = createOutputFile(hasGenericInterfaces) + const newSourceFile = createOutputFile(hasGenericInterfaces, sourceFile.getProject())Also applies to: 93-95
src/input-handler.ts (1)
68-73: Optional: detect tsconfig automatically for filePath flowsIf you want automatic target detection without a caller-supplied Project, consider constructing Project with a nearby tsconfig.json (e.g., walk up from filePath and use new Project({ tsConfigFilePath })).
tests/utils/compiler-config.test.ts (2)
44-51: Add a case for ESNext to lock intended semantics.determineScriptTarget currently returns the provided target verbatim. If ESNext should map to Latest, add an assertion and (if desired) implement mapping. Otherwise, clarify in docs that ESNext is preserved.
Example:
const compilerOptions: ts.CompilerOptions = { target: ts.ScriptTarget.ESNext } compilerConfig.initializeFromCompilerOptions(compilerOptions) expect(compilerConfig.getScriptTarget()).toBe(ts.ScriptTarget.ESNext)
60-69: Consider initializing from Project via the public helper in tests.To mirror production usage, it’s slightly clearer to use initializeCompilerConfig(project) here.
-compilerConfig.initializeFromProject(project) +initializeCompilerConfig(project)docs/compiler-configuration.md (2)
31-37: Tighten wording and note reserved-words scope.
- Minor copy tweak.
- Clarify that identifier validation is character-level; reserved-word checks occur elsewhere.
-When no explicit target is specified: +When no explicit target is specified: - Falls back to `ts.ScriptTarget.Latest` -- Provides maximum compatibility with modern JavaScript features +- Provides broad compatibility with modern JavaScript features - Can be overridden per-project as needed + +Note: Identifier validation uses `ts.isIdentifierStart/Part` (character-level) and does not enforce reserved words (e.g., `class`, `function`). Reserved-word checks are performed in downstream phases.
39-47: Document initialization order and per-project concurrency caveat.Since the config is a process-global singleton, concurrent projects can contend. Recommend calling initializeCompilerConfig(project) before generation and avoiding parallel invocations with different projects.
Add a short snippet:
import { initializeCompilerConfig } from '.../utils/compiler-config' // Before any generation using `project` initializeCompilerConfig(project) // ... proceed with codegenOptionally mention a future improvement: store per-Project targets in a WeakMap to avoid global contention.
tests/utils/identifier-utils.test.ts (4)
49-56: Runtime-target test is fine but still ASCII; include a Unicode probe.Add at least one non-ASCII assertion to ensure runtime target actually matters.
compilerConfig.setScriptTarget(ts.ScriptTarget.ES5) expect(isValidIdentifier('π')).toBe(true)
58-65: Explicit target override: add a negative example for ES3.To prove override works, include an ES3-specific failure.
compilerConfig.setScriptTarget(ts.ScriptTarget.ES5) expect(isValidIdentifier('π', ts.ScriptTarget.ES3)).toBe(false)
67-73: Edge cases look good. Consider ZWJ/ZWNJ validity.ECMAScript allows U+200C/200D in IdentifierPart. Add:
expect(isValidIdentifier('a\u200Cb')).toBe(true) // ZWNJ expect(isValidIdentifier('a\u200Db')).toBe(true) // ZWJ
75-80: Test name contradicts expectations; rename for clarity.You expect true for reserved words, so rename to reflect character-level checking only.
-test('should reject reserved words as identifiers', () => { +test('should not reject reserved words at the character-check stage', () => {tests/integration/script-target-integration.test.ts (3)
3-4: Import initializeCompilerConfig and use it before generation.Make the dependency explicit to avoid relying on hidden side effects inside generateCode.
-import { CompilerConfig } from '@daxserver/validation-schema-codegen/utils/compiler-config' +import { CompilerConfig, initializeCompilerConfig } from '@daxserver/validation-schema-codegen/utils/compiler-config'
49-56: Consider test.each for independent subtests and add ES3 to the matrix.for-of keeps this as one test; parameterized tests give clearer reporting and allow adding ES3 to verify Unicode differences.
- test('should work with different script targets', () => { - const testCases = [ - ts.ScriptTarget.ES5, - ts.ScriptTarget.ES2015, - ts.ScriptTarget.ES2020, - ts.ScriptTarget.Latest - ] - for (const target of testCases) { + test.each([ + ts.ScriptTarget.ES3, + ts.ScriptTarget.ES5, + ts.ScriptTarget.ES2015, + ts.ScriptTarget.ES2020, + ts.ScriptTarget.Latest, + ])('should work with script target %p', (target) => { const project = new Project({ compilerOptions: { target } }) + initializeCompilerConfig(project) // ... - } - }) + })
86-116: Numeric and underscored keys: good coverage.This is a useful sanity check; keeping initializeCompilerConfig(project) here too will make it robust.
const project = new Project({ compilerOptions: { target: ts.ScriptTarget.Latest } }) +initializeCompilerConfig(project)tests/handlers/typebox/enums.test.ts (1)
140-163: Add edge cases for enum initializers (optional)Consider adding:
- Mixed numeric + string members (e.g., B = 0, C = 'c') to assert stable handling.
- Non-first explicit initializer (e.g., B, C = 'c', D).
src/utils/identifier-utils.ts (1)
14-19: Handle Unicode code points correctly; prefer ts.isIdentifierTextThe current loop uses charCodeAt, which breaks on surrogate pairs (e.g., emoji, astral letters). Let TypeScript handle this.
Apply this diff:
- // First character must be valid identifier start - if (!ts.isIdentifierStart(text.charCodeAt(0), target)) return false - - // Remaining characters must be valid identifier parts - for (let i = 1; i < text.length; i++) { - if (!ts.isIdentifierPart(text.charCodeAt(i), target)) return false - } - - return true + // Use TS helper to handle surrogate pairs and escapes correctly + return ts.isIdentifierText(text, target)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
docs/compiler-configuration.md(1 hunks)docs/handler-system.md(1 hunks)docs/overview.md(1 hunks)src/handlers/typebox/object/object-like-base-handler.ts(3 hunks)src/index.ts(1 hunks)src/input-handler.ts(2 hunks)src/utils/compiler-config.ts(1 hunks)src/utils/identifier-utils.ts(1 hunks)tests/handlers/typebox/enums.test.ts(2 hunks)tests/handlers/typebox/interface-generics-consistency.test.ts(0 hunks)tests/handlers/typebox/interface-generics-runtime-binding.test.ts(4 hunks)tests/handlers/typebox/objects.test.ts(1 hunks)tests/import-resolution.test.ts(1 hunks)tests/input-handler.test.ts(1 hunks)tests/integration/script-target-integration.test.ts(1 hunks)tests/traverse/dependency-ordering.test.ts(3 hunks)tests/traverse/dependency-traversal.integration.test.ts(1 hunks)tests/traverse/simple-dependency.test.ts(1 hunks)tests/utils/compiler-config.test.ts(1 hunks)tests/utils/identifier-utils.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- tests/handlers/typebox/interface-generics-consistency.test.ts
🧰 Additional context used
🧬 Code graph analysis (12)
tests/handlers/typebox/enums.test.ts (1)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
src/utils/identifier-utils.ts (1)
src/utils/compiler-config.ts (2)
getScriptTarget(40-42)getScriptTarget(76-78)
tests/utils/compiler-config.test.ts (1)
src/utils/compiler-config.ts (4)
CompilerConfig(6-71)getScriptTarget(40-42)getScriptTarget(76-78)initializeCompilerConfig(83-85)
tests/utils/identifier-utils.test.ts (2)
src/utils/compiler-config.ts (1)
CompilerConfig(6-71)src/utils/identifier-utils.ts (1)
isValidIdentifier(8-22)
src/input-handler.ts (1)
src/utils/compiler-config.ts (1)
initializeCompilerConfig(83-85)
tests/handlers/typebox/objects.test.ts (1)
tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
tests/traverse/dependency-traversal.integration.test.ts (1)
tests/utils.ts (1)
createSourceFile(12-14)
tests/integration/script-target-integration.test.ts (2)
src/utils/compiler-config.ts (1)
CompilerConfig(6-71)tests/utils.ts (3)
createSourceFile(12-14)generateFormattedCode(26-37)formatWithPrettier(16-24)
tests/traverse/simple-dependency.test.ts (1)
tests/utils.ts (1)
createSourceFile(12-14)
src/handlers/typebox/object/object-like-base-handler.ts (1)
src/utils/identifier-utils.ts (1)
isValidIdentifier(8-22)
src/index.ts (1)
src/utils/compiler-config.ts (1)
initializeCompilerConfig(83-85)
tests/handlers/typebox/interface-generics-runtime-binding.test.ts (1)
tests/utils.ts (2)
createSourceFile(12-14)generateFormattedCode(26-37)
🪛 LanguageTool
docs/compiler-configuration.md
[grammar] ~35-~35: There might be a mistake here.
Context: ...t target is specified: - Falls back to ts.ScriptTarget.Latest - Provides maximum compatibility with mode...
(QB_NEW_EN)
docs/handler-system.md
[grammar] ~35-~35: There might be a mistake here.
Context: ....getText() and preserved as identifiers - **String literals** ('prop-name', "prop...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...d validated for identifier compatibility - Numeric literals (123) - extracted u...
(QB_NEW_EN)
🔇 Additional comments (17)
tests/import-resolution.test.ts (1)
80-80: No-op formatting change. LGTM.tests/traverse/simple-dependency.test.ts (1)
2-2: Good switch to shared test fixture.Centralizing source-file creation improves consistency and reduces duplication.
Also applies to: 10-19
tests/traverse/dependency-ordering.test.ts (3)
83-87: Property-key quoting normalization looks correct.Emitting unquoted identifiers for valid keys while retaining quotes for hyphenated keys matches the new identifier policy.
99-101: Formatting-only tweak.No behavioral change; ordering test remains valid.
121-134: Formatting-only tweak.No behavioral change; dependency chain assertions remain intact.
tests/handlers/typebox/interface-generics-runtime-binding.test.ts (1)
12-12: TS version 5.9.2 ≥ 4.7: instantiation expressions supported
TypeScript is pinned to ~5.9.2, which meets the ≥ 4.7 requirement for instantiation expressions—no changes needed.docs/overview.md (1)
49-49: LGTM: index entry addedGood cross-link to the new compiler configuration docs.
tests/input-handler.test.ts (1)
270-277: LGTM: newline-preservation testThe tweak ensures fidelity when reading from disk.
src/input-handler.ts (1)
68-73: Initialization placement looks goodRunning initializeCompilerConfig right after obtaining the Project ensures downstream consumers see the intended scriptTarget.
tests/utils/compiler-config.test.ts (4)
8-15: Global singleton + parallel tests can race across files.CompilerConfig is process-global; running test files in parallel may cause cross-file interference. Either serialize tests or scope config per project.
Suggested mitigations:
- Set Bun’s workers to 1 for this repo’s tests, or mark these suites as sequential.
- Alternatively, have tests call initializeCompilerConfig(project) and avoid relying on ambient global state.
71-77: LGTM — default fallback path is covered.Good assertion that missing target falls back to Latest.
80-96: Convenience helpers coverage looks solid.getScriptTarget and initializeCompilerConfig are exercised against distinct targets.
3-3: No action needed: path aliases are correctly configured and resolvable under bun:test.tsconfig.json defines
• “@daxserver/validation-schema-codegen” → “./src/index.ts”
• “@daxserver/validation-schema-codegen/” → “./src/”bun:test uses tsconfig paths by default, and no bunfig.toml overrides are required.
tests/utils/identifier-utils.test.ts (2)
18-25: Good baseline coverage.ASCII-valid identifiers are covered.
27-34: LGTM for common invalid forms.Covers empty, leading digit, punctuation, space, and symbol.
tests/handlers/typebox/enums.test.ts (1)
63-86: Mixed implicit/explicit enum value case — LGTMCovers the mixed initializer scenario and expected schema emission correctly.
tests/handlers/typebox/objects.test.ts (1)
55-84: Comprehensive property-name coverage — LGTMValidates identifiers, quoted names, spaces, numeric keys, and normalization of simple quoted identifiers.
| Object property names are extracted using the TypeScript compiler API through `PropertySignature.getNameNode()`. The system handles different property name formats: | ||
|
|
||
| - **Identifiers** (`prop`) - extracted using `nameNode.getText()` and preserved as identifiers | ||
| - **String literals** (`'prop-name'`, `"prop name"`) - extracted using `nameNode.getLiteralValue()` and validated for identifier compatibility | ||
| - **Numeric literals** (`123`) - extracted using `nameNode.getLiteralValue().toString()` and treated as identifiers | ||
|
|
||
| The system uses TypeScript's built-in character validation utilities (`ts.isIdentifierStart` and `ts.isIdentifierPart`) with runtime-determined script targets to determine if property names can be safely used as unquoted identifiers in the generated code. The script target is automatically determined from the ts-morph Project's compiler options, ensuring compatibility with the target JavaScript environment while maintaining optimal output format. | ||
|
|
There was a problem hiding this comment.
Correctness and clarity: numeric key handling and list punctuation
- Numeric keys aren’t “treated as identifiers”; they’re emitted as numeric literals.
- Minor grammar/punctuation fixes for list items.
Apply this diff:
-Object property names are extracted using the TypeScript compiler API through `PropertySignature.getNameNode()`. The system handles different property name formats:
+Object property names are extracted using the TypeScript compiler API via `PropertySignature.getNameNode()`. The system handles different property name formats:
-- **Identifiers** (`prop`) - extracted using `nameNode.getText()` and preserved as identifiers
-- **String literals** (`'prop-name'`, `"prop name"`) - extracted using `nameNode.getLiteralValue()` and validated for identifier compatibility
-- **Numeric literals** (`123`) - extracted using `nameNode.getLiteralValue().toString()` and treated as identifiers
+- **Identifiers** (`prop`): extracted using `nameNode.getText()` and preserved as identifiers.
+- **String literals** (`'prop-name'`, `"prop name"`): extracted using `nameNode.getLiteralValue()` and validated for identifier compatibility; emitted unquoted when safe, otherwise quoted.
+- **Numeric literals** (`123`): extracted using `nameNode.getLiteralValue().toString()` and emitted as numeric literals.
-The system uses TypeScript's built-in character validation utilities (`ts.isIdentifierStart` and `ts.isIdentifierPart`) with runtime-determined script targets to determine if property names can be safely used as unquoted identifiers in the generated code. The script target is automatically determined from the ts-morph Project's compiler options, ensuring compatibility with the target JavaScript environment while maintaining optimal output format.
+The system uses TypeScript's built-in identifier validation (`ts.isIdentifierStart`, `ts.isIdentifierPart`) with a runtime-determined script target to decide if property names can be safely emitted as unquoted identifiers. The script target is automatically derived from the ts-morph Project's compiler options, ensuring compatibility with the target JavaScript environment while maintaining optimal output format.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Object property names are extracted using the TypeScript compiler API through `PropertySignature.getNameNode()`. The system handles different property name formats: | |
| - **Identifiers** (`prop`) - extracted using `nameNode.getText()` and preserved as identifiers | |
| - **String literals** (`'prop-name'`, `"prop name"`) - extracted using `nameNode.getLiteralValue()` and validated for identifier compatibility | |
| - **Numeric literals** (`123`) - extracted using `nameNode.getLiteralValue().toString()` and treated as identifiers | |
| The system uses TypeScript's built-in character validation utilities (`ts.isIdentifierStart` and `ts.isIdentifierPart`) with runtime-determined script targets to determine if property names can be safely used as unquoted identifiers in the generated code. The script target is automatically determined from the ts-morph Project's compiler options, ensuring compatibility with the target JavaScript environment while maintaining optimal output format. | |
| Object property names are extracted using the TypeScript compiler API via `PropertySignature.getNameNode()`. The system handles different property name formats: | |
| - **Identifiers** (`prop`): extracted using `nameNode.getText()` and preserved as identifiers. | |
| - **String literals** (`'prop-name'`, `"prop name"`): extracted using `nameNode.getLiteralValue()` and validated for identifier compatibility; emitted unquoted when safe, otherwise quoted. | |
| - **Numeric literals** (`123`): extracted using `nameNode.getLiteralValue().toString()` and emitted as numeric literals. | |
| The system uses TypeScript's built-in identifier validation (`ts.isIdentifierStart`, `ts.isIdentifierPart`) with a runtime-determined script target to decide if property names can be safely emitted as unquoted identifiers. The script target is automatically derived from the ts-morph Project's compiler options, ensuring compatibility with the target JavaScript environment while maintaining optimal output format. |
🧰 Tools
🪛 LanguageTool
[grammar] ~35-~35: There might be a mistake here.
Context: ....getText() and preserved as identifiers - **String literals** ('prop-name', "prop...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...d validated for identifier compatibility - Numeric literals (123) - extracted u...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/handler-system.md around lines 33 to 40, update the wording to correctly
state that numeric property keys are emitted as numeric literals (not “treated
as identifiers”) and adjust the list item punctuation/grammar: change the
“Numeric literals (`123`) - extracted using
`nameNode.getLiteralValue().toString()` and treated as identifiers” line to
indicate they are emitted as numeric literals (e.g., extracted via
`getLiteralValue()` and emitted as numbers), and clean up punctuation on all
three list items so each item is a parallel, grammatically consistent sentence
with proper commas/periods.
| // Numeric properties can be used as identifiers | ||
| propName = nameNode.getLiteralValue().toString() | ||
| shouldUseIdentifier = true | ||
| } else { |
There was a problem hiding this comment.
Bug: numeric keys emitted as Identifier nodes
Creating an Identifier with text "123" is invalid; numeric keys must be a NumericLiteral node. This can lead to malformed AST and potential printer/syntax issues.
Apply this diff:
- } else if (Node.isNumericLiteral(nameNode)) {
- // Numeric properties can be used as identifiers
- propName = nameNode.getLiteralValue().toString()
- shouldUseIdentifier = true
+ } else if (Node.isNumericLiteral(nameNode)) {
+ // Preserve numeric keys as numeric literals
+ return ts.factory.createNumericLiteral(nameNode.getLiteralValue().toString())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Numeric properties can be used as identifiers | |
| propName = nameNode.getLiteralValue().toString() | |
| shouldUseIdentifier = true | |
| } else { | |
| } else if (Node.isNumericLiteral(nameNode)) { | |
| // Preserve numeric keys as numeric literals | |
| return ts.factory.createNumericLiteral(nameNode.getLiteralValue().toString()) | |
| } else { |
🤖 Prompt for AI Agents
In src/handlers/typebox/object/object-like-base-handler.ts around lines 54 to
57, numeric object keys are currently treated as Identifier nodes which is
invalid (e.g., "123" must be a NumericLiteral). Change the logic so when
nameNode.getLiteralValue() is numeric (or represents a numeric string), do not
mark shouldUseIdentifier true; instead keep shouldUseIdentifier false and ensure
the emitter creates a NumericLiteral node (using the literal value as a number)
for the property name; preserve the string propName only for non-numeric
identifiers.
| import { TypeBoxPrinter } from '@daxserver/validation-schema-codegen/printer/typebox-printer' | ||
| import { DependencyTraversal } from '@daxserver/validation-schema-codegen/traverse/dependency-traversal' | ||
| import type { TraversedNode } from '@daxserver/validation-schema-codegen/traverse/types' | ||
| import { initializeCompilerConfig } from '@daxserver/validation-schema-codegen/utils/compiler-config' |
There was a problem hiding this comment.
Don’t reinitialize CompilerConfig with a blank Project (overwrites detected target)
This call can reset a previously detected scriptTarget to Latest, breaking target-aware logic.
-import { initializeCompilerConfig } from '@daxserver/validation-schema-codegen/utils/compiler-config'
@@
- const project = new Project()
-
- initializeCompilerConfig(project)
+ const project = new Project()Also applies to: 13-17
🤖 Prompt for AI Agents
In src/index.ts around lines 8 and 13-17, the call to initializeCompilerConfig
that creates a blank Project is overwriting a previously detected scriptTarget
(often setting it to Latest) and breaking target-aware logic; remove the
blank-Project reinitialization (or change the call to accept the existing
Project/tsconfig or merge with detected compilerOptions) so the detected
scriptTarget is preserved, and ensure any defaulting only applies when no
project/options were previously detected.
| test('should use project compiler options for script target', () => { | ||
| const project = new Project({ | ||
| compilerOptions: { | ||
| target: ts.ScriptTarget.ES2015 | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize CompilerConfig from the Project.
Ensures the script target for this project is authoritative, even if tests run in parallel.
const project = new Project({
compilerOptions: {
target: ts.ScriptTarget.ES2015
}
})
+
+initializeCompilerConfig(project)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should use project compiler options for script target', () => { | |
| const project = new Project({ | |
| compilerOptions: { | |
| target: ts.ScriptTarget.ES2015 | |
| } | |
| }) | |
| test('should use project compiler options for script target', () => { | |
| const project = new Project({ | |
| compilerOptions: { | |
| target: ts.ScriptTarget.ES2015 | |
| } | |
| }) | |
| initializeCompilerConfig(project) |
🤖 Prompt for AI Agents
In tests/integration/script-target-integration.test.ts around lines 18 to 23,
the test constructs a Project with compilerOptions.target set but does not
initialize the test's CompilerConfig from that Project, so the project's script
target may be ignored when tests run in parallel; update the test to create a
CompilerConfig (or equivalent config object used by the code under test) from
the Project instance and use that CompilerConfig for any compilation/transform
steps in the test so the project's ScriptTarget is authoritative for this test.
| const sourceFile = createSourceFile(project, | ||
| ` | ||
| interface TestInterface { | ||
| validName: string; | ||
| "invalid-name": number; | ||
| } | ||
| ` | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a Unicode-only identifier to assert target-aware behavior.
Validate that non-ASCII letters are quoted for ES3 and unquoted for ES5+.
const sourceFile = createSourceFile(project,
`
interface TestInterface {
validName: string;
"invalid-name": number;
+ π: boolean;
}
`
)And adapt expectations per target accordingly (quoted π under ES3).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sourceFile = createSourceFile(project, | |
| ` | |
| interface TestInterface { | |
| validName: string; | |
| "invalid-name": number; | |
| } | |
| ` | |
| ) | |
| const sourceFile = createSourceFile(project, | |
| ` | |
| interface TestInterface { | |
| validName: string; | |
| "invalid-name": number; | |
| π: boolean; | |
| } | |
| ` | |
| ) |
🤖 Prompt for AI Agents
In tests/integration/script-target-integration.test.ts around lines 64 to 71,
add a Unicode-only identifier (e.g. π) to the TestInterface declaration and
update the test expectations so the generated output asserts target-aware
behavior: for ES3 expect the π member to be quoted, and for ES5+ expect the π
member to be unquoted; modify the test's expected strings per target accordingly
to validate quoted π under ES3 and unquoted π for ES5 and newer.
tests/utils/identifier-utils.test.ts
Outdated
| import { describe, expect, beforeEach, afterEach, test } from 'bun:test' | ||
| import { ts } from 'ts-morph' | ||
| import { isValidIdentifier } from '@daxserver/validation-schema-codegen/utils/identifier-utils' | ||
| import { CompilerConfig } from '@daxserver/validation-schema-codegen/utils/compiler-config' | ||
|
|
||
| describe('identifier-utils', () => { | ||
| let compilerConfig: CompilerConfig | ||
|
|
||
| beforeEach(() => { | ||
| compilerConfig = CompilerConfig.getInstance() | ||
| compilerConfig.reset() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| compilerConfig.reset() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential correctness gap in implementation: astral-plane code points.
isValidIdentifier uses charCodeAt over code units. Astral characters (e.g., 𝒜 U+1D49C) will be split into surrogate pairs and misclassified.
Proposed implementation (in src/utils/identifier-utils.ts):
export const isValidIdentifier = (text: string, scriptTarget?: ts.ScriptTarget): boolean => {
if (text.length === 0) return false
const target = scriptTarget ?? getScriptTarget()
let i = 0
// first code point
{
const cp = text.codePointAt(i)!
if (!ts.isIdentifierStart(cp, target)) return false
i += cp > 0xFFFF ? 2 : 1
}
while (i < text.length) {
const cp = text.codePointAt(i)!
if (!ts.isIdentifierPart(cp, target)) return false
i += cp > 0xFFFF ? 2 : 1
}
return true
}Add tests:
expect(isValidIdentifier('𝒜', ts.ScriptTarget.ES5)).toBe(true) // Mathematical script capital A
expect(isValidIdentifier('A𝒜', ts.ScriptTarget.ES5)).toBe(true)🤖 Prompt for AI Agents
In tests/utils/identifier-utils.test.ts lines 1-16: the implementation of
isValidIdentifier currently uses charCodeAt which misclassifies astral-plane
characters (surrogate pairs); update the function in
src/utils/identifier-utils.ts to iterate by Unicode code points (use
String.prototype.codePointAt and advance the index by 1 or 2 depending on cp >
0xFFFF), call ts.isIdentifierStart on the first code point and
ts.isIdentifierPart on subsequent code points, preserve the empty-string check
and optional scriptTarget handling, and add unit tests asserting that astral
identifiers like '𝒜' and 'A𝒜' are valid with ts.ScriptTarget.ES5.
tests/utils/identifier-utils.test.ts
Outdated
| test('should handle Unicode identifiers based on script target', () => { | ||
| // Test with explicit script target | ||
| expect(isValidIdentifier('validName', ts.ScriptTarget.ES5)).toBe(true) | ||
| expect(isValidIdentifier('validName', ts.ScriptTarget.ES2015)).toBe(true) | ||
| expect(isValidIdentifier('validName', ts.ScriptTarget.Latest)).toBe(true) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
This “Unicode” test only exercises ASCII; add true Unicode and ES3 variance.
Strengthen by asserting non-ASCII behavior and ES3 vs ES5 differences.
Add cases like:
// ES3 disallows non-ASCII starts; ES5+ allows many Unicode letters
expect(isValidIdentifier('π', ts.ScriptTarget.ES3)).toBe(false)
expect(isValidIdentifier('π', ts.ScriptTarget.ES5)).toBe(true)
expect(isValidIdentifier('名', ts.ScriptTarget.ES5)).toBe(true)
expect(isValidIdentifier('éclair', ts.ScriptTarget.ES5)).toBe(true)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/typebox/literal-type-handler.ts (1)
13-28: Support negative numeric literal types (-123).LiteralTypeNode for negatives is a PrefixUnaryExpression; currently falls through to Any.
switch (literal.getKind()) { case SyntaxKind.StringLiteral: return makeTypeCall('Literal', [ ts.factory.createStringLiteral(literal.getText().slice(1, -1)), ]) case SyntaxKind.NumericLiteral: return makeTypeCall('Literal', [ts.factory.createNumericLiteral(literal.getText())]) + case SyntaxKind.PrefixUnaryExpression: { + const prefix = literal.asKind(SyntaxKind.PrefixUnaryExpression) + if (prefix?.getOperatorToken() === SyntaxKind.MinusToken) { + const operand = prefix.getOperand() + if (Node.isNumericLiteral(operand)) { + return makeTypeCall('Literal', [ + ts.factory.createPrefixUnaryExpression( + ts.SyntaxKind.MinusToken, + ts.factory.createNumericLiteral(operand.getText()), + ), + ]) + } + } + break + } case SyntaxKind.TrueKeyword: return makeTypeCall('Literal', [ts.factory.createTrue()]) case SyntaxKind.FalseKeyword: return makeTypeCall('Literal', [ts.factory.createFalse()]) case SyntaxKind.NullKeyword: return makeTypeCall('Null') default: return makeTypeCall('Any') }
🧹 Nitpick comments (13)
src/handlers/typebox/template-literal-type-handler.ts (1)
33-36: Empty tail semantics: intentional to drop ''?
literal.textcan be empty for the final tail; the truthy check skips emitting an empty string literal. If TypeBox’sTemplateLiteralexpects an explicit trailing "" entry to preserve alternating structure, push it unconditionally; otherwise, add a brief comment stating that empty tails are intentionally omitted.Would you like me to add a small test matrix for cases like
${T},x${T},${T}x, andx${T}yto lock this in?src/handlers/typebox/literal-type-handler.ts (2)
15-17: Use getLiteralText() instead of slicing quotes.Avoid manual slice; it’s brittle for escapes. Prefer the API that returns the raw string value.
- return makeTypeCall('Literal', [ - ts.factory.createStringLiteral(literal.getText().slice(1, -1)), - ]) + return makeTypeCall('Literal', [ + ts.factory.createStringLiteral(literal.getLiteralText()), + ])
26-27: Avoid silent downgrade to Any.Returning Any hides unsupported cases. Prefer Unknown or surface a diagnostic.
- default: - return makeTypeCall('Any') + default: + return makeTypeCall('Unknown') + // or: throw new Error(`Unsupported literal kind: ${SyntaxKind[literal.getKind()]}`)src/handlers/typebox/reference/type-reference-base-handler.ts (1)
2-2: Handle qualified names robustly and use idiomatic type guards.
- Prefer
Node.isTypeReferencefor clarity.getTypeNameshould handleQualifiedName(e.g.,NS.Foo) to avoid mismatches whensupportedTypeNamescontains unqualified names.- canHandle(node: Node): boolean { - if (node.getKind() !== SyntaxKind.TypeReference) return false + canHandle(node: Node): boolean { + if (!Node.isTypeReference(node)) return false- protected getTypeName(node: TypeReferenceNode): string { - return node.getTypeName().getText() - } + protected getTypeName(node: TypeReferenceNode): string { + const tn = node.getTypeName() + return Node.isQualifiedName(tn) ? tn.getRight().getText() : tn.getText() + }Optional: if some handlers accept fewer-than-declared generic args (defaults), relax exact-arity validation or improve the error to include the type name.
Also applies to: 9-9, 25-35, 37-39
src/utils/node-type-utils.ts (1)
32-36: Support qualified type names in helpers.Current checks return false for
QualifiedName(e.g.,NS.Foo). Compare usinggetText()so both identifiers and qualified names work.- const typeNameNode = node.getTypeName() - - return Node.isIdentifier(typeNameNode) && typeNameNode.getText() === typeName + const nameText = node.getTypeName().getText() + return nameText === typeName- const typeNameNode = node.getTypeName() - - return Node.isIdentifier(typeNameNode) && typeNames.includes(typeNameNode.getText()) + const nameText = node.getTypeName().getText() + return typeNames.includes(nameText)Optional: if you intend to accept unqualified matches, also allow
nameText.endsWith('.' + typeName).Also applies to: 43-47
src/handlers/typebox/indexed-access-type-handler.ts (3)
17-19: Broaden typeof handling to qualified names.
typeof A.B[number]isn’t recognized because onlyIdentifieris handled. Consider supportingQualifiedNameby usingexprName.getText()and resolving via the type checker rather thangetTypeAlias/getVariableDeclarationby name.If helpful, I can propose a small utility that resolves
EntityNameto a declaration using the project’s TypeChecker.
86-90: Include numeric tuple literals in the union.Tuple unions currently include only string literals; add numeric support.
- if (literal.isKind(ts.SyntaxKind.StringLiteral)) { - const stringLiteral = literal.asKindOrThrow(ts.SyntaxKind.StringLiteral) - const value = stringLiteral.getLiteralValue() - literalTypes.push(makeTypeCall('Literal', [ts.factory.createStringLiteral(value)])) - } + if (literal.isKind(ts.SyntaxKind.StringLiteral)) { + const value = literal.asKindOrThrow(ts.SyntaxKind.StringLiteral).getLiteralValue() + literalTypes.push(makeTypeCall('Literal', [ts.factory.createStringLiteral(value)])) + } else if (literal.isKind(ts.SyntaxKind.NumericLiteral)) { + const num = Number(literal.asKindOrThrow(ts.SyntaxKind.NumericLiteral).getText()) + literalTypes.push(makeTypeCall('Literal', [ts.factory.createNumericLiteral(num)])) + }
56-61: Optional: handleas constarrays fortypeof A[number].If a variable lacks a type node but is
const A = ['a','b'] as const, consider inspecting the initializer andas constto build the same union, rather than falling back toIndex.I can draft a helper that extracts literal unions from variable initializers with
as const.src/utils/compiler-config.ts (1)
44-46: Optional: allow full reset between projects/tests.If you need a clean slate (e.g., in tests), consider clearing the instance as well.
reset(): void { this.scriptTarget = ts.ScriptTarget.Latest + // Optionally clear the singleton instance to force re-init on next access. + // CompilerConfig.instance = null }src/utils/identifier-utils.ts (1)
9-36: Prefer ts.isIdentifierText for correctness and simplicity.
ts.isIdentifierText(text, target)handles Unicode and reserved-word rules across ES versions; it’s less error-prone than per-code-point checks and aligns with emitter semantics.export const isValidIdentifier = (text: string): boolean => { - if (text.length === 0) return false - - const target = getScriptTarget() - - // First character must be valid identifier start - const firstCodePoint = text.codePointAt(0) - if (firstCodePoint === undefined || !ts.isIdentifierStart(firstCodePoint, target)) { - return false - } - - // Remaining characters must be valid identifier parts - // Use for...of to properly iterate over Unicode code points - let isFirst = true - for (const char of text) { - if (isFirst) { - isFirst = false - continue - } - - const codePoint = char.codePointAt(0) - if (codePoint === undefined || !ts.isIdentifierPart(codePoint, target)) { - return false - } - } - - return true + return text.length > 0 && ts.isIdentifierText(text, getScriptTarget()) }docs/compiler-configuration.md (1)
7-7: Nit: end sentence with a period.-The system automatically determines the appropriate TypeScript script target from the ts-morph Project's compiler options +The system automatically determines the appropriate TypeScript script target from the ts-morph Project's compiler options.src/handlers/typebox/collection/array-type-handler.ts (1)
12-15: Minor: inline temporary for brevity (optional)Slightly shorter without losing clarity.
- const typeboxType = getTypeBoxType(node.getElementTypeNode()) - - return makeTypeCall('Array', [typeboxType]) + return makeTypeCall('Array', [getTypeBoxType(node.getElementTypeNode())])src/utils/typebox-call.ts (1)
14-14: Fail-fast now throws on unknown types; improve error context and test reset hook.Add contextual info to the thrown error and expose a test-only reset for the module-level singleton.
Apply:
- return handlers.getHandler(node).handle(node) + try { + return handlers.getHandler(node).handle(node) + } catch (err) { + const kind = typeof (node as any).getKindName === 'function' ? (node as any).getKindName() : String(node.getKind?.()) + const preview = typeof node.getText === 'function' ? node.getText().slice(0, 120).replace(/\s+/g, ' ') : '' + throw new Error(`TypeBox handler resolution failed for ${kind}${preview ? `: "${preview}"` : ''}. ${err instanceof Error ? err.message : String(err)}`) + }Additionally (outside the shown range), add:
export const __resetTypeBoxHandlersForTests = () => { handlers = null }And verify no lingering uses of the removed
TypeBoxStatic:#!/bin/bash rg -nP '\bTypeBoxStatic\b'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (37)
docs/compiler-configuration.md(1 hunks)docs/utilities.md(1 hunks)src/handlers/typebox/collection/array-type-handler.ts(2 hunks)src/handlers/typebox/collection/collection-base-handler.ts(0 hunks)src/handlers/typebox/date-type-handler.ts(1 hunks)src/handlers/typebox/indexed-access-type-handler.ts(4 hunks)src/handlers/typebox/keyof-type-handler.ts(0 hunks)src/handlers/typebox/literal-type-handler.ts(1 hunks)src/handlers/typebox/object/interface-type-handler.ts(2 hunks)src/handlers/typebox/object/object-like-base-handler.ts(3 hunks)src/handlers/typebox/readonly-array-type-handler.ts(0 hunks)src/handlers/typebox/reference/omit-type-handler.ts(1 hunks)src/handlers/typebox/reference/pick-type-handler.ts(1 hunks)src/handlers/typebox/reference/type-reference-base-handler.ts(3 hunks)src/handlers/typebox/template-literal-type-handler.ts(1 hunks)src/handlers/typebox/type/keyof-type-handler.ts(1 hunks)src/handlers/typebox/type/readonly-array-type-handler.ts(1 hunks)src/handlers/typebox/type/type-operator-base-handler.ts(2 hunks)src/handlers/typebox/typebox-type-handlers.ts(2 hunks)src/input-handler.ts(2 hunks)src/parsers/parse-function-declarations.ts(1 hunks)src/parsers/parse-interfaces.ts(1 hunks)src/parsers/parse-type-aliases.ts(1 hunks)src/traverse/dependency-traversal.ts(0 hunks)src/traverse/file-graph.ts(1 hunks)src/traverse/node-graph.ts(2 hunks)src/traverse/types.ts(1 hunks)src/utils/add-static-type-alias.ts(1 hunks)src/utils/compiler-config.ts(1 hunks)src/utils/generic-type-utils.ts(1 hunks)src/utils/identifier-utils.ts(1 hunks)src/utils/interface-processing-order.ts(3 hunks)src/utils/node-type-utils.ts(3 hunks)src/utils/typebox-call.ts(1 hunks)tests/integration/script-target-integration.test.ts(1 hunks)tests/utils/compiler-config.test.ts(1 hunks)tests/utils/identifier-utils.test.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/handlers/typebox/readonly-array-type-handler.ts
- src/handlers/typebox/keyof-type-handler.ts
- src/traverse/dependency-traversal.ts
- src/handlers/typebox/collection/collection-base-handler.ts
✅ Files skipped from review due to trivial changes (7)
- src/handlers/typebox/reference/omit-type-handler.ts
- src/parsers/parse-type-aliases.ts
- src/handlers/typebox/reference/pick-type-handler.ts
- src/utils/interface-processing-order.ts
- src/utils/generic-type-utils.ts
- src/parsers/parse-interfaces.ts
- docs/utilities.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/input-handler.ts
- tests/utils/compiler-config.test.ts
- tests/integration/script-target-integration.test.ts
- src/handlers/typebox/object/object-like-base-handler.ts
- tests/utils/identifier-utils.test.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/handlers/typebox/template-literal-type-handler.ts (1)
src/handlers/typebox/template-literal-type-processor.ts (1)
TemplateLiteralTypeProcessor(7-58)
src/handlers/typebox/type/readonly-array-type-handler.ts (1)
src/handlers/typebox/readonly-array-type-handler.ts (1)
ReadonlyArrayTypeHandler(5-12)
src/handlers/typebox/type/type-operator-base-handler.ts (2)
src/utils/typebox-call.ts (1)
getTypeBoxType(7-15)src/utils/typebox-codegen-utils.ts (1)
makeTypeCall(3-12)
src/utils/typebox-call.ts (2)
src/utils/typebox-codegen-utils.ts (1)
makeTypeCall(3-12)src/handlers/typebox/typebox-type-handlers.ts (1)
TypeBoxTypeHandlers(28-145)
src/utils/identifier-utils.ts (1)
src/utils/compiler-config.ts (2)
getScriptTarget(30-32)getScriptTarget(52-54)
src/handlers/typebox/type/keyof-type-handler.ts (1)
src/handlers/typebox/keyof-type-handler.ts (1)
KeyOfTypeHandler(5-12)
src/handlers/typebox/object/interface-type-handler.ts (1)
src/utils/typebox-codegen-utils.ts (1)
makeTypeCall(3-12)
src/handlers/typebox/indexed-access-type-handler.ts (2)
src/utils/typebox-call.ts (1)
getTypeBoxType(7-15)src/utils/typebox-codegen-utils.ts (1)
makeTypeCall(3-12)
🪛 LanguageTool
docs/compiler-configuration.md
[grammar] ~35-~35: There might be a mistake here.
Context: ...t target is specified: - Falls back to ts.ScriptTarget.Latest - Provides maximum compatibility with mode...
(QB_NEW_EN)
🔇 Additional comments (19)
src/handlers/typebox/template-literal-type-handler.ts (1)
29-31: Confirmed TS version floor and safe removal of null-check on span.type
Declared TypeScript version is ~5.9.2 (> 4.1), andTemplateLiteralTypeSpan.typeis non-optional in the TS AST, so unguarded access is valid.src/handlers/typebox/literal-type-handler.ts (2)
3-3: LGTM on ts-morph imports.Importing ts via ts-morph is consistent with the rest of the file’s usage.
10-10: Confirm override safety
BaseTypeHandler declaresabstract handle(node: Node): ts.Expressionin src/handlers/typebox/base-type-handler.ts:5. Your override
handle(node: LiteralTypeNode): ts.Expressionnarrows the parameter type. TypeScript permits this via bivariance on class methods, but verify that every call site is guarded (e.g. by canHandle) or else update the base signature to a generic form (for example,
handle<T extends Node>(node: T): ts.Expression) to guarantee contract compatibility.src/utils/compiler-config.ts (2)
52-54: LGTM: concise accessor wrapper.
59-61: LGTM: straightforward project-based initializer.src/utils/identifier-utils.ts (1)
1-1: Verify path alias resolution.Ensure the alias
@daxserver/validation-schema-codegen/*is mapped in tsconfig paths for all build/test environments; otherwise switch to a relative import to avoid resolution issues.src/traverse/file-graph.ts (1)
18-23: LGTM: guarded add prevents duplicate node errors.Matches the style used elsewhere and keeps Graphology happy.
src/traverse/node-graph.ts (2)
13-15: LGTM: add node only when absent.
30-36: LGTM: well-scoped edge addition guard.Prevents self-loops and duplicate edges.
src/handlers/typebox/type/keyof-type-handler.ts (1)
4-7: LGTM: clean migration to operator-driven TypeBox call; old import path removed All aggregators reference the newkeyof-type-handlerand no stale imports remain.src/handlers/typebox/collection/array-type-handler.ts (1)
2-3: LGTM: switch to centralized TypeBox codegen utilitiesUsing
getTypeBoxType+makeTypeCallkeeps array handling consistent with the new codegen path.src/traverse/types.ts (1)
5-5: No occurrences ofSupportedNodeTypein codebase—confirm whether to re-export alias for public API
rg -n '\bSupportedNodeType\b' -g '!**/dist/**'returned no matches, but you should manually verify if consumers rely on the type alias and, if so, re-export it as a deprecated alias.src/handlers/typebox/typebox-type-handlers.ts (3)
23-24: KeyOf/ReadonlyArray imports: LGTM.Paths align with the new type/ layout and match the fallback registration below.
109-111: Minor early-return refactor: LGTM.No behavior change; keeps the hot path lean.
16-16: Verification complete: no stale references to the old import path remain.src/utils/typebox-call.ts (1)
8-8: Defaulting no-node to Type.Any: LGTM.Keeps previous semantics explicit.
src/handlers/typebox/type/type-operator-base-handler.ts (2)
4-4: Switch to centralized makeTypeCall: LGTM.Reduces subclass boilerplate.
19-24: AllcreateTypeBoxCalloverrides removed
A global search found no occurrences ofcreateTypeBoxCallin the codebase, so it’s safe to remove the obsolete overrides.src/handlers/typebox/type/readonly-array-type-handler.ts (1)
4-7: Readonly array operator wiring: LGTM.Configuration matches
SyntaxKind.ReadonlyKeyword→Type.Readonly(...).
| handle(): ts.Expression { | ||
| return makeTypeCall('Date') | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm base signature: handler.handle now parameterless
Assuming BaseTypeHandler’s contract changed to handle(): ts.Expression. If not, this override will be incompatible.
Run:
🏁 Script executed:
#!/bin/bash
rg -nP "class\s+BaseTypeHandler\b.*?\bhandle\s*\(" -U -C2Length of output: 59
🏁 Script executed:
#!/bin/bash
# Locate the BaseTypeHandler class definition
rg -nP "class\s+BaseTypeHandler\b" -n -C3
# Inspect BaseTypeHandler for handle signature
rg -nP "BaseTypeHandler[^}]*handle\s*\(" -n -C3Length of output: 865
Ensure handle override matches BaseTypeHandler signature
In src/handlers/typebox/date-type-handler.ts, change
handle(): ts.Expression {
return makeTypeCall('Date')
}to
handle(node: Node): ts.Expression {
return makeTypeCall('Date')
}to satisfy
abstract handle(node: Node): ts.Expressionin BaseTypeHandler.
🤖 Prompt for AI Agents
In src/handlers/typebox/date-type-handler.ts around lines 12 to 14, the handle
method currently has no parameters but must match the BaseTypeHandler signature;
change the method signature to accept a single parameter node: Node (i.e.
handle(node: Node): ts.Expression) while keeping the body returning
makeTypeCall('Date') so the override matches abstract handle(node: Node):
ts.Expression.
| canHandle(node: Node): boolean { | ||
| return Node.isLiteralTypeNode(node) || Node.isTrueLiteral(node) || Node.isFalseLiteral(node) | ||
| } |
There was a problem hiding this comment.
Fix canHandle/handle mismatch (can cause runtime errors).
canHandle accepts True/False nodes, but handle expects a LiteralTypeNode and immediately calls getLiteral(). If a bare True/False node ever routes here, this will blow up.
Apply this diff to restrict canHandle to literal type nodes only:
- canHandle(node: Node): boolean {
- return Node.isLiteralTypeNode(node) || Node.isTrueLiteral(node) || Node.isFalseLiteral(node)
- }
+ canHandle(node: Node): boolean {
+ return Node.isLiteralTypeNode(node)
+ }🤖 Prompt for AI Agents
In src/handlers/typebox/literal-type-handler.ts around lines 6 to 8, canHandle
currently returns true for bare True/False literal nodes as well as
LiteralTypeNodes, but handle assumes a LiteralTypeNode and calls getLiteral(),
which will fail for bare True/False nodes; change canHandle to only return
Node.isLiteralTypeNode(node) (remove the checks for Node.isTrueLiteral and
Node.isFalseLiteral) so only LiteralTypeNodes are routed here and handle's
getLiteral() call is safe.
| const expression = ts.factory.createArrayLiteralExpression(allTypes, true) | ||
|
|
||
| return makeTypeCall('Composite', [ts.factory.createArrayLiteralExpression(allTypes, true)]) | ||
| return makeTypeCall('Composite', [expression]) | ||
| } |
There was a problem hiding this comment.
Bug: creating identifiers from generic/qualified type text yields invalid AST; use node-driven conversion instead.
ts.factory.createIdentifier(typeText) and regex-based parsing will break for:
- Qualified names (
NS.Foo) — identifier cannot contain dots. - Generic types (
Foo<Bar,Baz>) — not a valid identifier.
Replace withgetTypeBoxType(typeNode)to build correct expressions; drop the brittle regex helpers.
- const expression = ts.factory.createArrayLiteralExpression(allTypes, true)
+ const expression = ts.factory.createArrayLiteralExpression(allTypes, true)- const typeText = typeNode.getText()
- extendedTypes.push(
- this.parseGenericTypeCall(typeText) ?? ts.factory.createIdentifier(typeText),
- )
+ extendedTypes.push(getTypeBoxType(typeNode))Add import (outside the changed hunk):
import { getTypeBoxType } from '@daxserver/validation-schema-codegen/utils/typebox-call'Then remove parseGenericTypeCall and createTypeExpression helpers; they’re no longer needed and are incomplete (single type arg only).
Also applies to: 60-66
🤖 Prompt for AI Agents
In src/handlers/typebox/object/interface-type-handler.ts around lines 21-24 (and
also apply same change to lines ~60-66), the code builds TypeBox AST by creating
identifiers from type text and using regex parsing which fails for qualified
names (NS.Foo) and generics (Foo<Bar>)—replace those brittle helpers by
importing getTypeBoxType from
'@daxserver/validation-schema-codegen/utils/typebox-call' and use
getTypeBoxType(typeNode) to produce the proper expression nodes instead of
ts.factory.createIdentifier(typeText) or
parseGenericTypeCall/createTypeExpression; remove the parseGenericTypeCall and
createTypeExpression helpers entirely and update call sites to pass the original
type AST node into getTypeBoxType so qualified and generic types are correctly
converted.
| // Check if parameter is optional or required | ||
| if (!param.hasQuestionToken()) { | ||
| return paramType | ||
| } | ||
|
|
||
| return paramType | ||
| // Parameter is optional | ||
| return ts.factory.createCallExpression( | ||
| ts.factory.createPropertyAccessExpression( | ||
| ts.factory.createIdentifier('Type'), | ||
| ts.factory.createIdentifier('Optional'), | ||
| ), | ||
| undefined, | ||
| [paramType], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Treat default-initialized parameters as optional.
Parameters with defaults are optional at call sites; currently only ? is considered. Encode both to avoid over-restricting generated function schemas.
- // Check if parameter is optional or required
- if (!param.hasQuestionToken()) {
- return paramType
- }
+ // Check if parameter is optional or required
+ const isOptional =
+ param.hasQuestionToken() || param.getInitializer() !== undefined
+ if (!isOptional) {
+ return paramType
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if parameter is optional or required | |
| if (!param.hasQuestionToken()) { | |
| return paramType | |
| } | |
| return paramType | |
| // Parameter is optional | |
| return ts.factory.createCallExpression( | |
| ts.factory.createPropertyAccessExpression( | |
| ts.factory.createIdentifier('Type'), | |
| ts.factory.createIdentifier('Optional'), | |
| ), | |
| undefined, | |
| [paramType], | |
| ) | |
| // Check if parameter is optional or required | |
| const isOptional = | |
| param.hasQuestionToken() || param.getInitializer() !== undefined | |
| if (!isOptional) { | |
| return paramType | |
| } | |
| // Parameter is optional | |
| return ts.factory.createCallExpression( | |
| ts.factory.createPropertyAccessExpression( | |
| ts.factory.createIdentifier('Type'), | |
| ts.factory.createIdentifier('Optional'), | |
| ), | |
| undefined, | |
| [paramType], | |
| ) |
🤖 Prompt for AI Agents
In src/parsers/parse-function-declarations.ts around lines 24 to 37, the code
only treats parameters with a question token as optional; update the logic to
also treat parameters that have a default initializer as optional. Specifically,
replace the current check so that a parameter is considered required only when
it has neither a question token nor an initializer, and wrap the paramType with
Type.Optional when either param.hasQuestionToken() or param.initializer is
present.
| const staticTypeNode = ts.factory.createTypeReferenceNode(ts.factory.createIdentifier('Static'), [ | ||
| ts.factory.createTypeQueryNode(ts.factory.createIdentifier(name)), | ||
| ]) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard import: ensure Static is type-imported from @sinclair/typebox in the generated file
Without importing Static, emitted export type ${name} = Static<typeof ${name}> won’t compile in consumers that don’t already have it in scope.
Apply this diff inside the function before adding the alias:
const staticType = printer.printNode(ts.EmitHint.Unspecified, staticTypeNode, compilerNode)
+ // Ensure `Static` type import exists
+ const hasStaticImport =
+ newSourceFile.getImportDeclarations().some(
+ (d) =>
+ d.getModuleSpecifierValue() === '@sinclair/typebox' &&
+ d.getNamedImports().some((n) => n.getName() === 'Static'),
+ )
+ if (!hasStaticImport) {
+ newSourceFile.addImportDeclaration({
+ isTypeOnly: true,
+ moduleSpecifier: '@sinclair/typebox',
+ namedImports: [{ name: 'Static' }],
+ })
+ }
+
newSourceFile.addTypeAlias({
isExported: true,
name,
type: staticType,
})🏁 Script executed:
#!/bin/bash
set -eux
file=$(fd -t f add-static-type-alias.ts)
echo "Found file: $file"
sed -n '1,200p' "$file"Length of output: 792
Add type-only import for Static
Insert a guard before adding the alias in src/utils/add-static-type-alias.ts to ensure consumers can resolve Static:
const staticType = printer.printNode(ts.EmitHint.Unspecified, staticTypeNode, compilerNode)
+ // Ensure `Static` type import exists
+ const hasStaticImport = newSourceFile
+ .getImportDeclarations()
+ .some(d =>
+ d.getModuleSpecifierValue() === '@sinclair/typebox' &&
+ d.getNamedImports().some(n => n.getName() === 'Static'),
+ )
+ if (!hasStaticImport) {
+ newSourceFile.addImportDeclaration({
+ isTypeOnly: true,
+ moduleSpecifier: '@sinclair/typebox',
+ namedImports: [{ name: 'Static' }],
+ })
+ }
newSourceFile.addTypeAlias({
isExported: true,
name,
type: staticType,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const staticTypeNode = ts.factory.createTypeReferenceNode(ts.factory.createIdentifier('Static'), [ | |
| ts.factory.createTypeQueryNode(ts.factory.createIdentifier(name)), | |
| ]) | |
| const staticType = printer.printNode(ts.EmitHint.Unspecified, staticTypeNode, compilerNode) | |
| // Ensure `Static` type import exists | |
| const hasStaticImport = newSourceFile | |
| .getImportDeclarations() | |
| .some(d => | |
| d.getModuleSpecifierValue() === '@sinclair/typebox' && | |
| d.getNamedImports().some(n => n.getName() === 'Static'), | |
| ) | |
| if (!hasStaticImport) { | |
| newSourceFile.addImportDeclaration({ | |
| isTypeOnly: true, | |
| moduleSpecifier: '@sinclair/typebox', | |
| namedImports: [{ name: 'Static' }], | |
| }) | |
| } | |
| newSourceFile.addTypeAlias({ | |
| isExported: true, | |
| name, | |
| type: staticType, | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
docs/compiler-configuration.md (2)
9-11: Clarify precedence and behavior when target is set to Latest.Explicitly state that if the Project sets target to ts.ScriptTarget.Latest, it is respected; environment detection is used only when target is undefined. This avoids ambiguity for consumers.
1. **Project Configuration** - Uses the target specified in the ts-morph Project's compiler options -2. **Environment Detection** - When no explicit target is found, detects the appropriate target based on the runtime TypeScript version +2. **Environment Detection** - When no explicit target is found, detects the appropriate target based on the runtime TypeScript version. + Note: If the Project sets `target` to `ts.ScriptTarget.Latest`, that setting is respected as-is.
43-55: Future-proof the TS→ES mapping note.TypeScript now periodically adds new ES targets (e.g., ES2024). Document that newer TS versions may map to newer targets, and note that the implementation checks for availability.
- **TypeScript 5.2+** → ES2023 - **TypeScript 5.0+** → ES2022 + (For newer TypeScript versions, the system may detect ES2024+ when available.)tests/utils/compiler-config.test.ts (1)
80-83: Align assertion with the comment or generalize the comment.Comment says “ES2023 or higher” but the assertion allows ES2020+. Either tighten the assertion for 5.2+ or soften the comment.
- // For TypeScript 5.9.2, should detect ES2023 or higher - expect(target).toBeGreaterThanOrEqual(ts.ScriptTarget.ES2020) + // Detects an environment-appropriate target (never Latest) + expect(target).toBeGreaterThanOrEqual(ts.ScriptTarget.ES2020)src/utils/compiler-config.ts (2)
6-13: Harden version parsing against atypical strings.ts.version can include prerelease/build tags. Your guard is mostly fine, but you can make parsing more explicit and avoid surprising fallbacks.
- const [major, minor] = ts.version.split('.').map(Number) + const [majorStr, minorStr] = ts.version.split('.') + const major = Number(majorStr?.replace(/\D+.*/, '')) + const minor = Number(minorStr?.replace(/\D+.*/, ''))
17-20: Optionally detect ES2024 when available (without breaking older TS).This keeps behavior optimal on newer TS while compiling on older TS versions.
- if (major >= 5) { - if (minor >= 2) return ts.ScriptTarget.ES2023 - if (minor >= 0) return ts.ScriptTarget.ES2022 - } + if (major >= 5) { + const ST: any = ts.ScriptTarget as any + // Prefer ES2024 if both TS and enum support it + if (minor >= 5 && typeof ST.ES2024 === 'number') return ST.ES2024 + if (minor >= 2) return ts.ScriptTarget.ES2023 + return ts.ScriptTarget.ES2022 + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/compiler-configuration.md(1 hunks)src/utils/compiler-config.ts(1 hunks)tests/utils/compiler-config.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/utils/compiler-config.test.ts (1)
src/utils/compiler-config.ts (4)
CompilerConfig(45-92)getScriptTarget(75-77)getScriptTarget(97-99)initializeCompilerConfig(104-106)
🪛 LanguageTool
docs/compiler-configuration.md
[grammar] ~43-~43: There might be a mistake here.
Context: ...version: - TypeScript 5.2+ → ES2023 - TypeScript 5.0+ → ES2022 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ... → ES2023 - TypeScript 5.0+ → ES2022 - TypeScript 4.9+ → ES2022 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ... → ES2022 - TypeScript 4.9+ → ES2022 - TypeScript 4.7+ → ES2021 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ... → ES2022 - TypeScript 4.7+ → ES2021 - TypeScript 4.5+ → ES2020 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ... → ES2021 - TypeScript 4.5+ → ES2020 - TypeScript 4.2+ → ES2019 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ... → ES2020 - TypeScript 4.2+ → ES2019 - TypeScript 4.1+ → ES2018 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ... → ES2019 - TypeScript 4.1+ → ES2018 - TypeScript 4.0+ → ES2017 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ... → ES2018 - TypeScript 4.0+ → ES2017 - TypeScript 3.8+ → ES2017 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~51-~51: There might be a mistake here.
Context: ... → ES2017 - TypeScript 3.8+ → ES2017 - TypeScript 3.6+ → ES2016 - **TypeScrip...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ... → ES2017 - TypeScript 3.6+ → ES2016 - TypeScript 3.4+ → ES2015 - **Older ver...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ... → ES2016 - TypeScript 3.4+ → ES2015 - Older versions → ES5 This ensures gen...
(QB_NEW_EN)
🔇 Additional comments (7)
docs/compiler-configuration.md (1)
22-28: Example reads well and matches behavior.The ES5-target illustration correctly shows identifier validation and quoting behavior.
tests/utils/compiler-config.test.ts (3)
53-62: Respecting Project target looks good.Initializing from a Project with ES2020 is correctly reflected in the config.
12-18: Ensure tests manipulating the shared CompilerConfig singleton are isolated or run serially. Tests in tests/utils/identifier-utils.test.ts, tests/utils/compiler-config.test.ts, and tests/integration/script-target-integration.test.ts all call CompilerConfig.getInstance() and reset it; if your test runner executes suites in parallel, they can race and cause flakiness. Confirm your runner’s concurrency settings or scope these mutations via per-suite helpers.
1-7: Verify module path alias resolution under Bun
Ensure your tsconfig.json includes apathsentry for"@daxserver/validation-schema-codegen/*"(e.g."@daxserver/validation-schema-codegen/*": ["src/*"]) and that Bun’s test runner is configured to use this tsconfig (via bunfig.toml or--tsconfig-override), otherwisebun testwill fail to resolve the import.src/utils/compiler-config.ts (3)
49-53: Singleton enforcement is correct.Private constructor + lazy getInstance() enforces the pattern.
68-70: Project initialization logic is sound.Prefers explicit project target and falls back to environment detection.
89-91: Reset behavior matches documented intent.Resets to environment-detected target; tests cover this.
1 participant
The changes in this commit introduce a new
CompilerConfigsingleton that manages the TypeScript compiler options and script target configuration for the codegen system.The key changes are:
Automatic script target detection: The system now automatically determines the appropriate TypeScript script target from the
ts-morphProject's compiler options.Identifier validation: Property names in generated TypeBox objects are validated using TypeScript's built-in utilities (
ts.isIdentifierStart()andts.isIdentifierPart()). This ensures compatibility with the detected script target.Configuration management: The
CompilerConfigsingleton provides a centralized way to manage the script target configuration, allowing it to be overridden per-project as needed.Default behavior: When no explicit target is specified, the system falls back to
ts.ScriptTarget.Latest, providing maximum compatibility with modern JavaScript features.Integration points: The configuration system is integrated with various components of the codegen system, including the Input Handler, Code Generation, Identifier Utils, and Object Handlers.
These changes improve the overall robustness and flexibility of the codegen system, ensuring that the generated code is compatible with the target JavaScript environment.