fix(json): support recursive definitions #1865
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to TypeScript compiler options and enhances the testing suite for JSON field handling. Specifically, it sets the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Outside diff range and nitpick comments (5)
packages/sdk/src/code-gen.ts (1)
18-18: Consider alternative approaches for recursive type supportInstead of disabling type checking, consider these type-safe alternatives:
- Use TypeScript's built-in recursive type syntax:
type JSONContent = { type: string; children?: JSONContent[]; text?: string; };
- Use interface declarations which naturally support recursion:
interface JSONContent { type: string; children?: JSONContent[]; text?: string; }
- Use type assertions or type parameters where needed:
type JSONContent<T = any> = { type: string; children?: Array<JSONContent<T>>; text?: string; };Would you like help implementing any of these alternative approaches?
tests/integration/tests/enhancements/json/typing.test.ts (2)
363-380: Consider expanding test coverageThe current test effectively validates the basic recursive structure. Consider adding test cases for:
- Edge cases with empty arrays and null fields
- Maximum nesting depth validation
- Complete field verification after retrieval
Example additional test case:
// Test edge cases const edgeCaseContent: Content = { type: 'text', content: [], // Empty array text: null // Null optional field }; // Test deep nesting const deepContent: Content = { type: 'root', content: Array(10).fill(null).map((_, i) => ({ type: `level-${i}`, text: i === 9 ? 'deepest' : undefined, content: [] })) }; // Verify all fields after retrieval const post = await db.post.create({ data: { content: edgeCaseContent } }); expect(post.content).toEqual(edgeCaseContent);
Line range hint
1-334: Consider reorganizing tests for better maintainabilityThe test suite is comprehensive but could benefit from better organization:
- Group related test cases using
describeblocks (e.g., "Basic Types", "Complex Types", "Recursive Types")- Extract common schema definitions into shared fixtures
- Consider using parameterized tests for type coverage scenarios
Example restructuring:
describe('JSON field typing', () => { describe('Basic Types', () => { it('works with simple field', ...); it('works with optional field', ...); it('works with array field', ...); }); describe('Complex Types', () => { it('works with type nesting', ...); it('works with enums', ...); }); describe('Recursive Types', () => { it('supports recursive definition', ...); it('handles edge cases', ...); }); });tests/integration/tests/enhancements/json/crud.test.ts (2)
348-391: Implementation successfully demonstrates recursive JSON type support.The test case effectively validates the core functionality of recursive JSON types, matching the requirements from issue #1858. The schema definition and nested data structure provide a good foundation.
Consider expanding test coverage to include:
- Invalid type structure validation
- Update operations on nested content
- Array manipulation (push/pop operations)
- Error cases (malformed data)
Example test cases:
// Invalid structure await expect( db.post.create({ data: { content: { type: 'text', content: [{ invalidField: true }] } } }) ).toBeRejectedByPolicy(); // Update nested content const updated = await db.post.update({ where: { id: post.id }, data: { content: { ...post.content, content: [...post.content.content, { type: 'text', text: 'new' }] } } });
390-390: Enhance assertion coverage for nested structures.The current assertion only checks a single nested property. Consider using a more comprehensive assertion that validates the entire structure.
// Replace the single property check with a full structure validation await expect(post).toMatchObject({ content: { type: 'text', content: [{ type: 'text', content: [{ type: 'text', text: 'hello' }] }] } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/sdk/src/code-gen.ts(1 hunks)tests/integration/tests/enhancements/json/crud.test.ts(1 hunks)tests/integration/tests/enhancements/json/typing.test.ts(1 hunks)
🔇 Additional comments (1)
tests/integration/tests/enhancements/json/typing.test.ts (1)
339-343: LGTM: Recursive type definition matches requirements
The Content type definition correctly implements the recursive structure requested in issue #1858, allowing for nested content through the optional array field while maintaining type safety.
| @@ -15,6 +15,7 @@ export function createProject(options?: CompilerOptions) { | |||
| strict: true, | |||
| skipLibCheck: true, | |||
| noEmitOnError: true, | |||
| noImplicitAny: false, | |||
There was a problem hiding this comment.
Reconsider disabling noImplicitAny as it reduces type safety
Setting noImplicitAny: false is a significant change that weakens TypeScript's type checking. While this may fix the recursive JSON type issue, it could:
- Allow untyped code to pass compilation
- Hide potential type errors
- Reduce code maintainability and type safety across the codebase
Consider keeping noImplicitAny: true and instead handle recursive types explicitly:
- noImplicitAny: false,Committable suggestion skipped: line range outside the PR's diff.
fixes #1858