feat(typescript): support inlined types (and initialize an AST) #4937
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focused on updating the 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: 22
🧹 Outside diff range and nitpick comments (15)
generators/typescript/codegen/src/typescript.ts (1)
17-17: LGTM! Consider grouping related exports for better readability.The export of
Referencefrom "./ast" is appropriate and aligns with the PR objectives. For improved code organization, consider grouping this export with the other named export from "./ast" (theType as Typesexport).Here's a suggested reorganization:
export * from "./ast"; -export { Type as Types } from "./ast"; +export { Type as Types, Reference } from "./ast"; -export { Reference } from "./ast"; export * from "./ast/core";generators/typescript/codegen/src/ast/core/AstNode.ts (1)
15-17: ApprovetoStringFormatted()with a suggestion for improvement.The
toStringFormatted()method is a useful addition, providing a formatted string representation of the AST node. However, theprettier.format()function typically requires options, especially the parser to use.Consider updating the
prettier.format()call to include options, particularly specifying the parser:public toStringFormatted(): string { return prettier.format(this.toString(), { parser: "typescript" }); }This ensures that the formatting is applied correctly for TypeScript code.
generators/typescript/codegen/src/ast/CodeBlock.ts (2)
9-11: LGTM: Class inheritance and constructor updated appropriately.The changes to the class inheritance and constructor are well-implemented:
- Extending
AbstractAstNodesuggests a refactoring of the class hierarchy, which could improve code organization or introduce shared functionality.- The new constructor signature with
private readonly argenhances encapsulation and immutability.Consider adding a brief comment explaining the reason for extending
AbstractAstNodeinstead ofAstNode. This would help future developers understand the design decision.
1-18: Overall assessment: Changes improve code structure and align with PR objectives.The modifications to
CodeBlock.tsare well-implemented and consistent. They contribute to the PR's goal of supporting inlined types and initializing an AST by:
- Refactoring the class hierarchy (extending
AbstractAstNode).- Enhancing encapsulation and immutability in the constructor.
- Maintaining consistent functionality while adapting to the new structure.
These changes appear to be part of a larger refactoring effort, which should improve the overall architecture of the TypeScript code generation system.
As this seems to be part of a broader refactoring:
- Ensure that similar changes are applied consistently across related classes.
- Consider updating or creating documentation that explains the new class hierarchy and the role of
AbstractAstNode.- If not already planned, consider creating a migration guide for any external code that might be affected by these changes.
🧰 Tools
🪛 Biome
[error] 16-16: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
generators/typescript/codegen/src/ast/__test__/Interface.test.ts (2)
75-88: LGTM: Great test for interface extension and module referencing.This test case effectively verifies the creation of an interface that extends another interface from a different module. The use of
ts.Reference.module()demonstrates proper handling of module imports and references.Consider adding a test case for an interface extending multiple interfaces to ensure the
extendsarray can handle multiple entries correctly.
1-90: Overall, excellent test suite with room for minor improvements.This test suite for the
interface_function is comprehensive and well-structured. It covers a wide range of scenarios for interface generation, from simple to complex cases. The use of snapshot testing is appropriate for verifying string outputs, and the gradual increase in complexity across test cases is a good testing strategy.To further enhance the test suite, consider the following suggestions:
- Add a test case for an interface extending multiple interfaces.
- Ensure the readonly properties test case correctly implements and verifies readonly functionality.
- Consider adding edge cases, such as interfaces with very long names or properties with special characters.
generators/typescript/express/generator/src/contexts/type/TypeContextImpl.ts (1)
128-129: LGTM with suggestions for improvementThe addition of
respectInlinedTypes: truealigns well with the PR objective of supporting inlined types in TypeScript. However, I have a few suggestions to enhance this change:
Consider adding a comment explaining the purpose and impact of the
respectInlinedTypesparameter. This will help other developers understand its significance.It might be beneficial to make the
respectInlinedTypesvalue configurable, rather than hardcoding it totrue. This could provide more flexibility for different use cases. For example:respectInlinedTypes: this.shouldRespectInlinedTypesWhere
shouldRespectInlinedTypescould be a class property or a parameter passed to the method.The addition of a trailing comma after
retainOriginalCasingis a good practice for version control and future code modifications.generators/typescript/model/type-generator/src/union/GeneratedUnionTypeImpl.ts (1)
32-33: LGTM! Consider a minor improvement for consistency.The changes to the constructor simplify its signature and maintain the same functionality. The use of
this.includeUtilsOnUnionMembersandthis.includeOtherInUnionTypessuggests these properties are now set in the parent class constructor or elsewhere in the class.For consistency, consider updating the
ParsedSingleUnionTypeForUnioninstantiation to usethis.includeUtilsOnUnionMembersinstead of accessinginit.includeUtilsOnUnionMembersdirectly:new ParsedSingleUnionTypeForUnion({ singleUnionType, union: this.shape, - includeUtilsOnUnionMembers: this.includeUtilsOnUnionMembers, + includeUtilsOnUnionMembers: this.includeUtilsOnUnionMembers, includeSerdeLayer: this.includeSerdeLayer, retainOriginalCasing: this.retainOriginalCasing, - noOptionalProperties: this.noOptionalProperties, + noOptionalProperties: this.noOptionalProperties, })This change would align with the usage pattern in the rest of the constructor.
Also applies to: 40-43, 51-52, 61-61
generators/typescript/model/type-generator/src/TypeGenerator.ts (3)
37-37: LGTM. Consider adding documentation for the new property.The addition of
respectInlinedTypesaligns with the PR objectives to support inlined types.Consider adding a JSDoc comment to explain the purpose and impact of this new property:
/** * When true, the generator will respect inlined types in the IR. * This affects how types are generated and referenced throughout the codebase. */ respectInlinedTypes: boolean;
50-50: LGTM. Consider adding documentation for the new property.The addition of
respectInlinedTypesto theArgsinterface is consistent with the change in theInitinterface.Consider adding a JSDoc comment to explain the purpose and impact of this new property:
/** * When true, the generator will respect inlined types for this specific type generation. * This affects how the current type is generated and referenced. */ respectInlinedTypes: boolean;
149-149: Good consistency in applying configuration across type generators.The use of
...this.initin all type generation methods ensures consistency and improves maintainability.Consider extracting a common method for creating type implementations to reduce code duplication:
private createTypeImpl<T extends new (args: any) => any>( Impl: T, args: ConstructorParameters<T>[0] ): InstanceType<T> { return new Impl({ ...args, ...this.init }); }Then use it in each method:
return this.createTypeImpl(GeneratedUnionTypeImpl, { typeName, shape, examples, docs, fernFilepath, getReferenceToSelf });Also applies to: 175-175, 202-202, 229-229, 238-238
generators/typescript/codegen/src/ast/Interface.ts (1)
42-42: Consider omitting quotes around property namesProperty names in interface definitions typically do not need to be enclosed in quotes unless they are not valid identifiers.
Consider updating the line to:
-writer.write(`"${property.name}"`); +writer.write(`${property.name}`);This will make the generated code cleaner unless there is a specific reason to include quotes.
generators/typescript/codegen/src/ast/Reference.ts (1)
30-55: Add unit tests for theReferenceclassAdding unit tests for the
Referenceclass will help ensure that its methods behave as expected, especially thewritemethod which handles different cases based onargs.type. This will improve code reliability and catch potential issues early.Would you like assistance in generating unit tests for this class or opening a GitHub issue to track this task?
generators/typescript/model/type-generator/src/AbstractGeneratedType.ts (1)
43-45: Add documentation for new constructor parametersThe constructor now includes
includeUtilsOnUnionMembersandincludeOtherInUnionTypesas parameters. Ensure that these parameters are adequately documented, explaining their purpose and how they affect the class behavior for better maintainability.generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (1)
18-42: Remove or update the commented-out codeLines 18 to 42 contain a large block of commented-out code, including the previous implementation of the
writeToFilemethod. Keeping extensive commented-out code can clutter the codebase and hinder maintainability. If this code is no longer needed, please remove it. If you plan to refactor or update it later, consider using version control to track changes instead of commenting out large sections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
generators/typescript/codegen/src/ast/__test__/__snapshots__/Interface.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (54)
- generators/typescript/codegen/package.json (1 hunks)
- generators/typescript/codegen/src/ast/CodeBlock.ts (1 hunks)
- generators/typescript/codegen/src/ast/Interface.ts (1 hunks)
- generators/typescript/codegen/src/ast/Reference.ts (1 hunks)
- generators/typescript/codegen/src/ast/Type.ts (3 hunks)
- generators/typescript/codegen/src/ast/test/Interface.test.ts (1 hunks)
- generators/typescript/codegen/src/ast/core/AstNode.ts (1 hunks)
- generators/typescript/codegen/src/ast/index.ts (1 hunks)
- generators/typescript/codegen/src/typescript.ts (1 hunks)
- generators/typescript/express/cli/package.json (1 hunks)
- generators/typescript/express/express-endpoint-type-schemas-generator/package.json (1 hunks)
- generators/typescript/express/express-error-generator/package.json (1 hunks)
- generators/typescript/express/express-error-schema-generator/package.json (1 hunks)
- generators/typescript/express/express-inlined-request-body-generator/package.json (1 hunks)
- generators/typescript/express/express-inlined-request-body-schema-generator/package.json (1 hunks)
- generators/typescript/express/express-register-generator/package.json (1 hunks)
- generators/typescript/express/express-service-generator/package.json (1 hunks)
- generators/typescript/express/generator/package.json (1 hunks)
- generators/typescript/express/generator/src/ExpressGenerator.ts (1 hunks)
- generators/typescript/express/generator/src/contexts/type-schema/TypeSchemaContextImpl.ts (1 hunks)
- generators/typescript/express/generator/src/contexts/type/TypeContextImpl.ts (1 hunks)
- generators/typescript/model/type-generator/package.json (1 hunks)
- generators/typescript/model/type-generator/src/AbstractGeneratedType.ts (4 hunks)
- generators/typescript/model/type-generator/src/TypeGenerator.ts (8 hunks)
- generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (1 hunks)
- generators/typescript/model/type-generator/src/union/GeneratedUnionTypeImpl.ts (2 hunks)
- generators/typescript/model/type-reference-converters/package.json (1 hunks)
- generators/typescript/model/type-reference-example-generator/package.json (1 hunks)
- generators/typescript/model/type-schema-generator/package.json (1 hunks)
- generators/typescript/model/union-generator/package.json (1 hunks)
- generators/typescript/model/union-schema-generator/package.json (1 hunks)
- generators/typescript/sdk/cli/package.json (1 hunks)
- generators/typescript/sdk/client-class-generator/package.json (1 hunks)
- generators/typescript/sdk/endpoint-error-union-generator/package.json (1 hunks)
- generators/typescript/sdk/environments-generator/package.json (1 hunks)
- generators/typescript/sdk/generator/package.json (1 hunks)
- generators/typescript/sdk/generator/src/SdkGenerator.ts (1 hunks)
- generators/typescript/sdk/generator/src/contexts/type-schema/TypeSchemaContextImpl.ts (1 hunks)
- generators/typescript/sdk/generator/src/contexts/type/TypeContextImpl.ts (1 hunks)
- generators/typescript/sdk/request-wrapper-generator/package.json (1 hunks)
- generators/typescript/sdk/sdk-endpoint-type-schemas-generator/package.json (1 hunks)
- generators/typescript/sdk/sdk-error-generator/package.json (1 hunks)
- generators/typescript/sdk/sdk-error-schema-generator/package.json (1 hunks)
- generators/typescript/sdk/sdk-inlined-request-body-schema-generator/package.json (1 hunks)
- generators/typescript/utils/abstract-generator-cli/package.json (1 hunks)
- generators/typescript/utils/commons/package.json (1 hunks)
- generators/typescript/utils/contexts/package.json (1 hunks)
- generators/typescript/utils/resolvers/package.json (1 hunks)
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/CreateMovieRequest.ts (1 hunks)
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/Movie.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/CreateMovieRequest.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/Movie.ts (1 hunks)
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/CreateMovieRequest.ts (1 hunks)
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/Movie.ts (1 hunks)
✅ Files skipped from review due to trivial changes (5)
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/Movie.ts
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/CreateMovieRequest.ts
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/Movie.ts
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/CreateMovieRequest.ts
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/Movie.ts
🧰 Additional context used
🪛 Biome
generators/typescript/codegen/src/ast/Type.ts
[error] 141-141: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 147-147: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 153-153: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 160-160: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 167-167: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (44)
generators/typescript/codegen/src/ast/index.ts (1)
5-6: New exports align with PR objectives.The addition of
InterfaceandReferenceexports is consistent with the PR's objective to support inlined types and initialize an AST. These new exports enhance the module's capabilities for handling TypeScript-specific constructs.To ensure these new exports are properly implemented and used, let's verify their existence and usage:
✅ Verification successful
New exports
InterfaceandReferenceare correctly added and present in the codebase. No issues found based on current usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of Interface.ts and Reference.ts files fd -e ts -d 1 'Interface|Reference' generators/typescript/codegen/src/ast # Check for usage of Interface and Reference in the codebase rg --type typescript 'import.*\{.*(?:Interface|Reference).*\}.*from.*["'\''].*ast.*["'\'']' generators/typescript/codegen/srcLength of output: 340
Script:
#!/bin/bash # Check for usage of Interface and Reference in the codebase rg --type ts --type tsx 'import.*\{.*(?:Interface|Reference).*\}.*from.*["'\''"].*ast.*["'\''"]' generators/typescript/codegen/srcLength of output: 160
Script:
#!/bin/bash # Check for usage of Interface and Reference in the codebase using glob patterns rg 'import.*\{.*(?:Interface|Reference).*\}.*from.*["'\''].*ast.*["'\'']' generators/typescript/codegen/src --glob '*.ts' --glob '*.tsx'Length of output: 138
generators/typescript/codegen/src/typescript.ts (2)
11-13: LGTM! Consistent implementation ofinterface_function.The new
interface_function follows the same pattern as the existingcodeblockandvariablefunctions, providing a convenient way to createAST.Interfaceinstances. The naming convention with an underscore is appropriate for avoiding conflicts with theinterfacekeyword in TypeScript.
Line range hint
1-19: Summary: Enhancements for AST support and inlined typesThe changes in this file successfully introduce support for interfaces in the AST and expose the necessary types for working with inlined types. The new
interface_function and the export ofReferenceare well-aligned with the PR objectives and follow the existing patterns in the file. These additions will enable better handling of TypeScript interfaces and references within the AST, contributing to the overall goal of supporting inlined types.generators/typescript/codegen/src/ast/core/AstNode.ts (2)
2-3: LGTM: Import statements are correct and necessary.The new import statements for
Writerandprettierare correctly placed and necessary for the newly added methods.
6-13: LGTM:toString()method is well-implemented.The
toString()method provides a clear and efficient way to convert the AST node to a string representation. It correctly utilizes theWriterclass and follows a common pattern for string conversion.generators/typescript/codegen/src/ast/CodeBlock.ts (2)
1-1: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
AbstractAstNode, which aligns with the changes in the class inheritance.
15-17: LGTM:writemethod updated correctly.The
writemethod has been properly updated to usethis.arginstead ofthis.args, which is consistent with the constructor changes.To ensure that this change doesn't introduce any regressions, please verify that all calls to
CodeBlock.write()still function as expected. You may want to run the existing test suite or add new tests specifically for this method.✅ Verification successful
Verification Successful:
CodeBlock.write()usage is confined toCodeBlock.ts. No additional impacts detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of CodeBlock.write() method rg --type typescript "CodeBlock.*write\(" generators/typescript/Length of output: 101
Script:
#!/bin/bash # Search for usages of CodeBlock.write() method in TypeScript files by extension rg "CodeBlock.*write\(" --glob "*.ts" --glob "*.tsx" generators/typescript/Length of output: 174
🧰 Tools
🪛 Biome
[error] 16-16: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
generators/typescript/codegen/package.json (1)
31-31: Approved: Dependency update for @fern-fern/ir-sdkThe update of
@fern-fern/ir-sdkto version 53.14.0 is approved. This change aligns with the PR objectives of supporting inlined types and initializing an AST.Please ensure the following:
- Thoroughly test the changes to verify compatibility with the rest of the codebase.
- Update the changelog if this dependency update introduces any breaking changes or new features.
- Verify that this update doesn't introduce any unexpected behavior in relation to the new inlined types support and AST initialization.
To assist in verifying the impact of this change, you can run the following script:
This script will help identify areas of the codebase that might be affected by the SDK update and any related TODOs or FIXMEs that developers might have left.
generators/typescript/sdk/environments-generator/package.json (1)
30-30: LGTM! Dependency update looks good.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is approved. This change keeps the project up-to-date with the latest features and bug fixes from the IR SDK.To ensure the update doesn't introduce any breaking changes or unexpected behavior, please:
- Run the test suite for this package and any dependent packages.
- Verify that the build process completes successfully.
- If there are any significant changes introduced by this update, consider updating the changelog.
You can use the following script to check for any breaking changes or deprecations in the new version:
generators/typescript/model/type-generator/package.json (1)
30-30: LGTM! Dependency update looks good.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is a minor version bump, which typically introduces new features or non-breaking changes. This aligns with the PR objective of enhancing TypeScript support and introducing inlined types handling.To ensure this update doesn't introduce any unexpected changes, please verify the following:
- Check the changelog for
@fern-fern/ir-sdkto understand what changes were introduced between versions 53.8.0 and 53.14.0.- Run your test suite to confirm that the update doesn't break any existing functionality.
- Review any TypeScript-related code that depends on this package to ensure it's compatible with the new version.
Would you like assistance in locating and reviewing the changelog for this update?
generators/typescript/sdk/request-wrapper-generator/package.json (1)
31-31: LGTM! Verify consistency across the project.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is a good practice to keep dependencies up-to-date. This minor version update should maintain backwards compatibility.To ensure consistency across the project, please run the following script to check if this dependency version is the same in all package.json files:
If any inconsistencies are found, consider updating all occurrences to maintain version uniformity across the project.
generators/typescript/express/express-service-generator/package.json (1)
30-30: LGTM. Verify consistency and impact of the dependency update.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is a minor version bump, which should maintain backwards compatibility. This update is likely to include new features, improvements, or bug fixes.To ensure consistency and assess potential impacts, please run the following script:
This script will help verify the consistency of the update across the project and identify any potential breaking changes or new features that may need to be addressed.
✅ Verification successful
Dependency update verified successfully.
All instances of
@fern-fern/ir-sdkhave been consistently updated to version53.14.0across the project. No pendingTODOorFIXMEcomments related to this update were found, and there are no associated breaking changes impacting the current functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency of @fern-fern/ir-sdk version across the project and look for any breaking changes or new features. # Test 1: Check for consistency of @fern-fern/ir-sdk version echo "Checking @fern-fern/ir-sdk version consistency:" rg --type json '"@fern-fern/ir-sdk":\s*"53\.14\.0"' -g 'package.json' # Test 2: Look for any mentions of breaking changes or new features in recent commits echo "Checking for mentions of breaking changes or new features:" git log -n 20 --grep="@fern-fern/ir-sdk" --grep="breaking change" --grep="new feature" --oneline # Test 3: Check if there are any TODOs or FIXMEs related to this update echo "Checking for TODOs or FIXMEs related to the update:" rg --type typescript "TODO|FIXME" -g "*.ts" | rg "@fern-fern/ir-sdk"Length of output: 3811
Script:
#!/bin/bash # Description: Check for consistency of @fern-fern/ir-sdk version across the project and look for any breaking changes or new features. # Test 1: Check for consistency of @fern-fern/ir-sdk version echo "Checking @fern-fern/ir-sdk version consistency:" rg --type json '"@fern-fern/ir-sdk":\s*"53\.14\.0"' -g 'package.json' # Test 2: Look for any mentions of breaking changes or new features in recent commits echo "Checking for mentions of breaking changes or new features:" git log -n 20 --grep="@fern-fern/ir-sdk" --grep="breaking change" --grep="new feature" --oneline # Test 3: Check if there are any TODOs or FIXMEs related to this update echo "Checking for TODOs or FIXMEs related to the update:" rg --type ts "TODO|FIXME" -g "*.ts" | rg "@fern-fern/ir-sdk"Length of output: 3768
generators/typescript/sdk/sdk-error-generator/package.json (1)
30-30: LGTM! Verify compatibility with the project.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is approved. This minor version bump likely includes new features and improvements.To ensure smooth integration, please verify the compatibility of this new version with the rest of the project. Run the following script to check for any potential issues:
generators/typescript/express/express-error-generator/package.json (1)
30-30: LGTM! Dependency update approved.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 looks good. This change aligns with the PR objectives of enhancing TypeScript support and introducing inlined types handling.To ensure this update doesn't introduce any breaking changes, please make sure to:
- Run the project's test suite.
- Verify that the project builds successfully.
- Check for any deprecation warnings or new features that might affect the current implementation.
Would you like me to generate a script to help verify the impact of this update?
generators/typescript/sdk/sdk-inlined-request-body-schema-generator/package.json (1)
30-30: Dependency update looks good, but verify consistency and test.The update of @fern-fern/ir-sdk from 53.8.0 to 53.14.0 is a minor version change, which should be backwards compatible. This update is likely to include new features or bug fixes that could benefit the project.
To ensure consistency across the project and identify any potential issues, please run the following verification steps:
After verifying consistency, please ensure to test the package thoroughly with the new dependency version to catch any potential compatibility issues.
generators/typescript/express/express-inlined-request-body-schema-generator/package.json (1)
30-30: LGTM: Dependency version update looks good.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is in line with the PR objectives to support inlined types and initialize an AST for TypeScript.To ensure this update doesn't introduce any breaking changes, please run the following verification script:
If the script doesn't find any breaking changes or TypeScript errors, the update should be safe to proceed.
generators/typescript/sdk/sdk-error-schema-generator/package.json (1)
31-31: LGTM: Dependency version update looks good.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is in line with the PR objectives of supporting inlined types and initializing an AST for TypeScript. This minor version update likely introduces the necessary features for these enhancements.To ensure compatibility, please run the following script to check for any breaking changes or deprecations in the changelog between these versions:
If the script returns any results, please review them carefully to ensure that they don't negatively impact your project.
generators/typescript/sdk/endpoint-error-union-generator/package.json (1)
30-30: Approved: Dependency update looks good, but verify its impact.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is a minor version change, which should maintain backward compatibility. This update is likely to include new features, improvements, or bug fixes.To ensure this update doesn't introduce any unexpected issues:
- Check if this update is consistent across other related packages in the project.
- Review the changelog or release notes for
@fern-fern/ir-sdkto understand what changes are included in this update.- Run the project's test suite to verify that the update doesn't break any existing functionality.
This script will help identify any inconsistencies in the
@fern-fern/ir-sdkversion across the project and list anypackage.jsonfiles that haven't been updated to the latest version.generators/typescript/sdk/client-class-generator/package.json (1)
31-31: LGTM! Dependency update looks good.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is approved. This change aligns with the PR objectives to enhance TypeScript support and introduce inlined types handling.To ensure this update is consistent across the project and to identify any potential impacts, please run the following verification script:
This script will help ensure that:
- The version update is consistent across all package.json files.
- There are no outstanding TODOs or FIXMEs related to this update.
- We can see where @fern-fern/ir-sdk is being imported, which might need attention due to the update.
Also, don't forget to:
- Update the changelog if necessary.
- Run the test suite to ensure this update doesn't introduce any regressions.
Let me know if you need any assistance in interpreting the results or making further changes.
generators/typescript/express/express-register-generator/package.json (1)
30-30: LGTM! Verify compatibility with the updated dependency.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 looks good. This change aligns with the PR objectives of enhancing TypeScript support.To ensure compatibility, please verify that this update doesn't introduce any breaking changes. You can check the changelog or release notes of
@fern-fern/ir-sdkfor versions 53.9.0 through 53.14.0.✅ Verification successful
Dependency update verified successfully.
All
package.jsonfiles have been updated to use@fern-fern/ir-sdkversion53.14.0. No references to53.8.0were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the @fern-fern/ir-sdk dependency is consistently updated across all package.json files # Test: Search for any package.json files that still reference the old version rg --type json '"@fern-fern/ir-sdk": "53.8.0"' -g 'package.json' # Test: Verify that all package.json files use the new version rg --type json '"@fern-fern/ir-sdk": "53.14.0"' -g 'package.json'Length of output: 3114
generators/typescript/express/express-error-schema-generator/package.json (1)
31-31: LGTM. Verify compatibility with the updated dependency.The update of @fern-fern/ir-sdk from version 53.8.0 to 53.14.0 is approved. This change keeps the project up-to-date with the latest features and bug fixes.
To ensure this update doesn't introduce any breaking changes or compatibility issues, please run the following verification steps:
After running these checks, please review the results to ensure there are no unexpected issues introduced by this dependency update.
generators/typescript/express/express-endpoint-type-schemas-generator/package.json (1)
31-31: LGTM! Verify compatibility with the new version.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 aligns with the PR objective of supporting inlined types and initializing an AST. This change is likely part of a broader update across multiple packages.To ensure compatibility, please run the following script to check for any breaking changes or deprecation notices in the changelog:
Also, make sure to run the project's test suite to catch any potential issues introduced by this update.
generators/typescript/utils/abstract-generator-cli/package.json (1)
34-34: Dependency update looks good, but verify changelog.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 aligns with the PR objectives of enhancing TypeScript support. This minor version bump likely includes new features and improvements that will benefit the project.To ensure a smooth integration, please verify the changelog of
@fern-fern/ir-sdkfor any breaking changes or important updates between versions 53.8.0 and 53.14.0. This will help identify any potential impacts on the project and necessary adjustments.generators/typescript/sdk/sdk-endpoint-type-schemas-generator/package.json (1)
31-31: LGTM. Verify consistency across the project.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 aligns with the PR objectives of enhancing TypeScript support and introducing inlined types. This minor version update suggests new features or non-breaking changes have been introduced.To ensure consistency, please run the following script to check if this dependency update has been applied uniformly across all relevant package.json files in the project:
If any inconsistencies are found, please update the other package.json files accordingly.
generators/typescript/express/cli/package.json (1)
34-34: LGTM! Verify compatibility with the updated dependency.The update of
@fern-fern/ir-sdkfrom version53.8.0to53.14.0looks good. This change aligns with the PR objectives to enhance TypeScript support.To ensure this update doesn't introduce any breaking changes, please run the following verification script:
generators/typescript/utils/commons/package.json (1)
34-34: Dependency update approved. Verify consistency across the project.The update of
@fern-fern/ir-sdkfrom version "53.8.0" to "53.14.0" looks good. This minor version bump likely includes new features or bug fixes that could benefit the project.To ensure consistency across the project, please run the following script to check if this dependency is updated to the same version in other package.json files:
If any inconsistencies are found, please update those files accordingly to maintain version uniformity across the project.
generators/typescript/express/generator/package.json (1)
32-32: Dependency update looks good, but verify changelog.The update of
@fern-fern/ir-sdkfrom version 53.8.0 to 53.14.0 is in line with the PR objectives. This update likely introduces new features or bug fixes that support inlined types and AST initialization.To ensure this update doesn't introduce any breaking changes, please verify the changelog for
@fern-fern/ir-sdkbetween versions 53.8.0 and 53.14.0. You can use the following script to check for any mentions of breaking changes:If the script returns any results, please review them carefully and assess their impact on this project.
generators/typescript/codegen/src/ast/__test__/Interface.test.ts (7)
5-11: LGTM: Well-structured test for a simple interface.This test case effectively checks the basic functionality of creating an empty interface. The use of snapshot testing is appropriate for verifying string outputs.
13-20: LGTM: Good test for exported interface.This test case properly verifies the creation of an exported interface. Testing the
exportflag is crucial as it affects the interface declaration.
22-28: LGTM: Effective test for interface with a single property.This test case appropriately checks the creation of an interface with a basic property. It's a logical progression from the previous tests, gradually increasing complexity.
30-40: LGTM: Comprehensive test for interface with multiple properties.This test case effectively verifies the creation of an interface with multiple properties of different types. It represents a more realistic use case for interfaces, which is crucial for thorough testing.
42-51: LGTM: Well-designed test for interface with optional properties.This test case properly verifies the creation of an interface with both required and optional properties. The use of
questionMark: trueto denote optional properties is correct and tests an important feature of TypeScript interfaces.
64-73: LGTM: Excellent test for interface with complex types.This test case effectively verifies the creation of an interface with more complex type definitions, including an array of strings and a literal type. Testing these complex types is crucial for ensuring the interface generator can handle various TypeScript type constructs accurately.
53-62: Verify readonly property implementation.This test case aims to check the creation of an interface with readonly properties. However, there's no explicit use of a readonly flag or modifier in the test setup. Please verify if the readonly functionality is implemented as intended or if there's a missing readonly modifier in the test case.
To confirm the implementation of readonly properties, you can run the following script:
generators/typescript/model/type-generator/src/TypeGenerator.ts (3)
59-61: Great refactoring of the constructor!The simplified constructor signature improves maintainability and reduces the risk of parameter mismatches. The initialization of existing properties ensures backward compatibility.
123-123: Good use of object spreading for configuration.Using
...this.initensures all configuration options are passed toGeneratedUndiscriminatedUnionTypeImpl, including any future additions to theInitinterface. This improves maintainability.
Line range hint
1-264: Overall, the changes effectively implement support for inlined types.The modifications to
TypeGeneratorconsistently add support for inlined types across all type generation methods. The code maintains good quality and consistency. Consider the suggested documentation additions and potential refactoring to further improve the code.generators/typescript/express/generator/src/contexts/type-schema/TypeSchemaContextImpl.ts (1)
107-108: LGTM! Consider verifying type generation behavior.The addition of
respectInlinedTypes: truealigns with the PR objective of supporting inlined types in TypeScript. This change should enhance the type generation process to handle inlined types correctly.To ensure the change doesn't introduce unexpected behavior:
- Verify that existing type generations still work as expected.
- Test with inlined types to confirm they are now properly supported.
- Update any relevant documentation or comments to reflect this new behavior.
Run the following script to check for any potential issues:
These checks will help ensure that the change is consistently applied and properly documented across the codebase.
generators/typescript/sdk/generator/src/SdkGenerator.ts (2)
301-302: LGTM: Support for inlined types addedThe addition of
respectInlinedTypes: trueto the TypeGenerator configuration enables support for inlined types. This change aligns well with the PR objective of enhancing TypeScript support for inlined types.
301-302: Verify impact on dependent codeWhile the change is localized to the TypeGenerator configuration, it's important to ensure that other parts of the codebase that interact with the TypeGenerator are compatible with this new behavior.
To verify the impact, please run the following script:
generators/typescript/model/type-generator/src/AbstractGeneratedType.ts (3)
5-5: Confirm the existence and correctness ofTypeGeneratorimportEnsure that the module
"./TypeGenerator"exists and exportsTypeGenerator. This is necessary for extendingTypeGenerator.Initin theInitinterface on line 8.
8-8: Validate the extension ofTypeGenerator.InitBy extending
TypeGenerator.Init, theInitinterface may now include additional properties. Verify that all inherited properties are correctly integrated intoAbstractGeneratedTypeand that there are no conflicts or unintended overrides with existing properties.
56-57: Assignments of new properties in the constructor are correctThe assignments of
includeUtilsOnUnionMembersandincludeOtherInUnionTypesto the class properties are properly handled, ensuring that the class uses the values provided during instantiation.generators/typescript/sdk/generator/src/contexts/type-schema/TypeSchemaContextImpl.ts (1)
107-108: Addition ofrespectInlinedTypesaligns with inlined types supportThe inclusion of
respectInlinedTypes: truein thegenerateTypemethod call ensures that inlined types are properly handled during type generation, aligning with the PR objective to support inlined types.
| @@ -27,7 +27,7 @@ | |||
| "depcheck": "depcheck" | |||
| }, | |||
| "dependencies": { | |||
| "@fern-fern/ir-sdk": "53.8.0", | |||
| "@fern-fern/ir-sdk": "53.14.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent @fern-fern/ir-sdk Versions Detected
The version of @fern-fern/ir-sdk is not consistent across all package.json files. Please update the following files to version 53.14.0:
- generators/pythonv2/codegen/package.json
- generators/ruby/sdk/package.json
- generators/ruby/model/package.json
- generators/ruby/codegen/package.json
- generators/ruby/cli/package.json
- generators/php/sdk/package.json
- generators/php/model/package.json
- generators/php/codegen/package.json
- generators/postman/package.json
- generators/openapi/package.json
- generators/csharp/sdk/package.json
- generators/csharp/model/package.json
- generators/csharp/codegen/package.json
After updating, please run the test suite to ensure all changes are compatible.
🔗 Analysis chain
Dependency update approved. Verify consistency and run tests.
The update of @fern-fern/ir-sdk from version 53.8.0 to 53.14.0 is approved. This change aligns with the PR objectives of enhancing TypeScript support and introducing inlined types.
To ensure consistency and catch potential issues:
-
Verify that this update is consistent across other package.json files in the project:
-
After updating dependencies, it's crucial to run the project's test suite to ensure no breaking changes were introduced. Please make sure to run:
npm test
or the equivalent command in your project's setup to verify that all tests pass with the updated dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of @fern-fern/ir-sdk version across package.json files
# Test: Search for @fern-fern/ir-sdk in all package.json files
# Expect: All occurrences should show version 53.14.0
rg --type json '"@fern-fern/ir-sdk"' -g 'package.json'
Length of output: 4015
| @@ -27,7 +27,7 @@ | |||
| "depcheck": "depcheck" | |||
| }, | |||
| "dependencies": { | |||
| "@fern-fern/ir-sdk": "53.8.0", | |||
| "@fern-fern/ir-sdk": "53.14.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Found in @fern-fern/ir-sdk Versions
The version of @fern-fern/ir-sdk is inconsistent across several package.json files:
-
Pythonv2 and PHP Packages:
generators/pythonv2/codegen/package.json: "^53.7.0"generators/php/sdk/package.json: "^53.7.0"generators/php/model/package.json: "^53.7.0"generators/php/codegen/package.json: "^53.7.0"generators/csharp/sdk/package.json: "^53.7.0"generators/csharp/codegen/package.json: "^53.7.0"generators/csharp/model/package.json: "^53.7.0"
-
Ruby Packages:
generators/ruby/sdk/package.json: "^39"generators/ruby/model/package.json: "^39"generators/ruby/cli/package.json: "^39"generators/ruby/codegen/package.json: "^39"
-
Other Packages:
generators/postman/package.json: "53.9.0"generators/openapi/package.json: "53.9.0"
Please update these to version 53.14.0 to maintain consistency across the project, unless there's a specific reason for the current versions.
🔗 Analysis chain
LGTM. Verify consistency across the project.
The update of @fern-fern/ir-sdk from version 53.8.0 to 53.14.0 is a good practice to keep the project up-to-date. This minor version update should introduce new features or non-breaking changes.
To ensure consistency, please run the following script to check if this update has been applied uniformly across all relevant package.json files in the project:
If any inconsistencies are found, please update the other package.json files accordingly or provide a rationale for the difference.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of @fern-fern/ir-sdk version across package.json files
# Test: Search for @fern-fern/ir-sdk dependency in all package.json files
# Expect: All occurrences should use version 53.14.0
rg --type json '"@fern-fern/ir-sdk":\s*"[^"]*"' -g 'package.json'
Length of output: 4026
| @@ -27,7 +27,7 @@ | |||
| "depcheck": "depcheck" | |||
| }, | |||
| "dependencies": { | |||
| "@fern-fern/ir-sdk": "53.8.0", | |||
| "@fern-fern/ir-sdk": "53.14.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistencies found in @fern-fern/ir-sdk versions across multiple package.json files.
The following files do not have @fern-fern/ir-sdk updated to version 53.14.0:
generators/pythonv2/codegen/package.jsongenerators/php/sdk/package.jsongenerators/php/codegen/package.json(appears multiple times)generators/postman/package.jsongenerators/openapi/package.jsongenerators/ruby/sdk/package.jsongenerators/ruby/codegen/package.jsongenerators/ruby/model/package.jsongenerators/ruby/cli/package.jsongenerators/csharp/sdk/package.jsongenerators/csharp/model/package.jsongenerators/csharp/codegen/package.json
Please update these files to ensure all parts of the project use @fern-fern/ir-sdk version 53.14.0 for consistency and to prevent potential dependency issues.
🔗 Analysis chain
Dependency update looks good, but let's verify consistency.
The update of @fern-fern/ir-sdk from version 53.8.0 to 53.14.0 is a minor version bump, which typically indicates new features or non-breaking changes. This change looks good.
To ensure consistency across the project, let's verify if this update has been applied to all relevant package.json files:
Also, please ensure that any new features or changes introduced in this version are properly utilized or accounted for in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of @fern-fern/ir-sdk version across all package.json files
# Test: Search for @fern-fern/ir-sdk in all package.json files
# Expect: All occurrences should show version 53.14.0
rg --type json '"@fern-fern/ir-sdk":\s*"([^"]*)"' -g 'package.json'
Length of output: 4028
| @@ -28,7 +28,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@fern-api/core-utils": "workspace:*", | |||
| "@fern-fern/ir-sdk": "53.8.0", | |||
| "@fern-fern/ir-sdk": "53.14.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent versions of @fern-fern/ir-sdk detected.
While the primary update to version 53.14.0 is present in most packages, some packages still reference older versions (53.9.0, ^53.7.0, ^39). To maintain consistency and avoid potential dependency conflicts, please update all instances to 53.14.0.
🔗 Analysis chain
Dependency update approved. Ensure thorough testing.
The update of @fern-fern/ir-sdk from version 53.8.0 to 53.14.0 is a minor version bump, which should be backwards compatible. This update is likely to include bug fixes and new features that could benefit the project.
To ensure this update doesn't introduce any unexpected issues:
- Please run the project's test suite and verify that all tests pass with the new dependency version.
- Check if this update is consistent across other related packages in the project.
You can use the following script to verify the consistency of this update across the project:
This script will help identify any inconsistencies in the @fern-fern/ir-sdk version across different package.json files in the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of @fern-fern/ir-sdk version across the project
# Search for @fern-fern/ir-sdk in all package.json files
echo "Searching for @fern-fern/ir-sdk versions in package.json files:"
rg --type json '"@fern-fern/ir-sdk":\s*"([^"]*)"' -g 'package.json'
# Count unique versions
echo -e "\nUnique versions found:"
rg --type json '"@fern-fern/ir-sdk":\s*"([^"]*)"' -g 'package.json' | sed -E 's/.*"([0-9.]+)".*/\1/' | sort | uniq -c
# Note: If multiple versions are found, consider updating all packages to use the same version.
Length of output: 5234
| @@ -30,7 +30,7 @@ | |||
| "@fern-api/logger": "workspace:*", | |||
| "@fern-api/generator-commons": "workspace:*", | |||
| "@fern-fern/generator-exec-sdk": "^0.0.898", | |||
| "@fern-fern/ir-sdk": "53.8.0", | |||
| "@fern-fern/ir-sdk": "53.14.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent versions of @fern-fern/ir-sdk found across the project.
Please update all package.json files to use version 53.14.0 of @fern-fern/ir-sdk to maintain consistency:
generators/postman/package.json: currently "53.9.0"generators/pythonv2/codegen/package.json: currently "^53.7.0"generators/ruby/sdk/package.json: currently "^39"generators/ruby/model/package.json: currently "^39"generators/ruby/codegen/package.json: currently "^39"generators/ruby/cli/package.json: currently "^39"generators/php/sdk/package.json: currently "^53.7.0"generators/php/model/package.json: currently "^53.7.0"generators/php/codegen/package.json: currently "^53.7.0"generators/openapi/package.json: currently "53.9.0"generators/csharp/sdk/package.json: currently "^53.7.0"generators/csharp/codegen/package.json: currently "^53.7.0"generators/csharp/model/package.json: currently "^53.7.0"
Ensuring all instances use the same version will help avoid potential compatibility issues.
🔗 Analysis chain
LGTM! Verify consistency across the project.
The update of @fern-fern/ir-sdk from version 53.8.0 to 53.14.0 is a good practice to keep dependencies up-to-date. This minor version bump should be backwards compatible according to semantic versioning principles.
To ensure consistency, let's verify if this update has been applied uniformly across the project:
If any inconsistencies are found, please update all occurrences to version 53.14.0 for uniformity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency of @fern-fern/ir-sdk version across the project
# Test: Search for @fern-fern/ir-sdk in all package.json files
# Expect: All occurrences should show version 53.14.0
rg --type json '"@fern-fern/ir-sdk":\s*"([^"]*)"' -g 'package.json'
Length of output: 4028
| import { AstNode } from "./core/AstNode"; | ||
| import { Writer } from "./core/Writer"; | ||
|
|
||
| export declare namespace Reference { |
There was a problem hiding this comment.
Avoid using the declare keyword in implementation files
The declare keyword is meant for ambient declarations in .d.ts files to describe external code. Since this is an implementation file, you should remove the declare keyword from the namespace declaration.
Apply this diff to fix the issue:
-export declare namespace Reference {
+export namespace Reference {📝 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.
| export declare namespace Reference { | |
| export namespace Reference { |
| protected includeUtilsOnUnionMembers: boolean; | ||
| protected includeOtherInUnionTypes: boolean; |
There was a problem hiding this comment.
Ensure new protected properties are properly initialized
The new protected properties includeUtilsOnUnionMembers and includeOtherInUnionTypes are introduced. Confirm that these properties are initialized in the constructor and have appropriate default values if necessary to prevent potential undefined issues during runtime.
| interface Number { | ||
| type: "number"; | ||
| } |
There was a problem hiding this comment.
Rename interfaces to avoid conflict with built-in types
Defining interfaces named Number, Date, Set, Map, and Undefined can cause confusion and potential conflicts with built-in types in TypeScript. Consider renaming these interfaces to avoid naming conflicts. For example:
Number➔Number_orNumberTypeDate➔Date_orDateTypeSet➔Set_orSetTypeMap➔Map_orMapTypeUndefined➔Undefined_orUndefinedType
Apply these diffs to rename the interfaces:
-interface Number {
+interface Number_ {
type: "number";
}
-interface Date {
+interface Date_ {
type: "date";
}
-interface Set {
+interface Set_ {
type: "set";
value: Type;
}
-interface Map {
+interface Map_ {
type: "map";
keyType: Type;
valueType: Type;
}
-interface Undefined {
+interface Undefined_ {
type: "undefined";
}Also, update all references to these interfaces throughout the code.
Also applies to: 32-34, 49-52, 59-63, 65-67
| return new this({ | ||
| type: "number" | ||
| }); | ||
| } |
There was a problem hiding this comment.
Replace this with class name Type in static methods
Using this in static methods can be confusing because this refers to the class constructor, not an instance. It's clearer to use the class name directly. In the static factory methods, replace new this({...}) with new Type({...}).
Apply these diffs to fix the issue:
For the number() method:
public static number(): Type {
- return new this({
+ return new Type({
type: "number"
});
}For the boolean() method:
public static boolean(): Type {
- return new this({
+ return new Type({
type: "bool"
});
}For the array(value: Type) method:
public static array(value: Type): Type {
- return new this({
+ return new Type({
type: "array",
value
});
}For the literal(value: string | boolean) method:
public static literal(value: string | boolean): Type {
- return new this({
+ return new Type({
type: "literal",
value
});
}For the reference(reference: Reference) method:
public static reference(reference: Reference): Type {
- return new this({
+ return new Type({
type: "reference",
reference
});
}Also applies to: 147-150, 153-157, 160-164, 167-171
🧰 Tools
🪛 Biome
[error] 141-141: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
| context.sourceFile.set({ | ||
| statements: "waddddupppp" |
There was a problem hiding this comment.
Implement the writeToFile method functionality
The current implementation sets the statements of context.sourceFile to the placeholder string "waddddupppp". This means the object type definition is not being generated as intended, which could lead to issues in code generation. Please replace the placeholder with the actual implementation that handles inlined types and initializes the AST.
If this is a work in progress, consider marking the pull request as a draft until the implementation is complete.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
generators/typescript/codegen/src/ast/__test__/__snapshots__/Interface.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
- generators/typescript/codegen/src/ast/Interface.ts (1 hunks)
- generators/typescript/codegen/src/ast/Reference.ts (1 hunks)
- generators/typescript/codegen/src/ast/core/Writer.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- generators/typescript/codegen/src/ast/Interface.ts
- generators/typescript/codegen/src/ast/Reference.ts
🧰 Additional context used
| const imports: string[] = []; | ||
| if (!skipImports) { | ||
| for (const reference of this.references) { | ||
| switch (reference.args.type) { | ||
| case "module": | ||
| imports.push(`import * as ${reference.args.module} from "${reference.args.source}";`); | ||
| break; | ||
| case "named": | ||
| imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return `${imports.join("\n")}\n\n${this.buffer}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider deduplicating import statements to prevent duplicates
If this.references contains multiple references to the same module or named import, the current implementation will generate duplicate import statements. This can lead to unnecessary code redundancy and potential issues during compilation.
Consider modifying the code to deduplicate the imports before generating them. You can use a Set to collect unique import strings.
Here's a suggested refactor:
public toString(skipImports = false): string {
- const imports: string[] = [];
+ const importSet: Set<string> = new Set();
if (!skipImports) {
for (const reference of this.references) {
switch (reference.args.type) {
case "module":
- imports.push(`import * as ${reference.args.module} from "${reference.args.source}";`);
+ importSet.add(`import * as ${reference.args.module} from "${reference.args.source}";`);
break;
case "named":
- imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`);
+ importSet.add(`import { ${reference.args.name}} from "${reference.args.source}";`);
break;
}
}
}
- return `${imports.join("\n")}\n\n${this.buffer}`;
+ return `${Array.from(importSet).join("\n")}\n\n${this.buffer}`;
}📝 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 imports: string[] = []; | |
| if (!skipImports) { | |
| for (const reference of this.references) { | |
| switch (reference.args.type) { | |
| case "module": | |
| imports.push(`import * as ${reference.args.module} from "${reference.args.source}";`); | |
| break; | |
| case "named": | |
| imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`); | |
| break; | |
| } | |
| } | |
| } | |
| return `${imports.join("\n")}\n\n${this.buffer}`; | |
| const importSet: Set<string> = new Set(); | |
| if (!skipImports) { | |
| for (const reference of this.references) { | |
| switch (reference.args.type) { | |
| case "module": | |
| importSet.add(`import * as ${reference.args.module} from "${reference.args.source}";`); | |
| break; | |
| case "named": | |
| importSet.add(`import { ${reference.args.name}} from "${reference.args.source}";`); | |
| break; | |
| } | |
| } | |
| } | |
| return `${Array.from(importSet).join("\n")}\n\n${this.buffer}`; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
generators/typescript/codegen/src/typescript.ts (1)
11-13: LGTM! Consider renaming for consistency.The
interface_function is well-implemented and follows the existing pattern in the file. It's a good addition for creatingAST.Interfaceinstances.Consider renaming
interface_tocreateInterfacefor consistency with TypeScript naming conventions and to avoid the trailing underscore. This would make it more aligned with common factory function naming patterns.generators/typescript/codegen/src/ast/core/Writer.ts (3)
2-8: LGTM! Consider encapsulatingisAmbient.The new import and property declarations look good. The
referencesarray is correctly set as private. However, consider makingisAmbientprivate with a getter method if direct external access isn't necessary. This would improve encapsulation and allow for future modifications to the property's behavior without changing the public API.If direct access to
isAmbientisn't required, consider refactoring like this:private _isAmbient: boolean = false; public get isAmbient(): boolean { return this._isAmbient; }This change would allow you to maintain the current functionality while improving encapsulation.
10-12: LGTM! Consider handling duplicate references.The
addReferencemethod correctly adds new references to thereferencesarray. However, it doesn't check for duplicates, which could lead to redundant import statements.Consider modifying the method to prevent duplicate references:
public addReference(reference: Reference): void { if (!this.references.some(ref => ref.args.type === reference.args.type && ref.args.source === reference.args.source && (ref.args.type === 'module' ? ref.args.module === reference.args.module : ref.args.name === reference.args.name) )) { this.references.push(reference); } }This change would ensure that each unique reference is only added once, potentially reducing redundant import statements.
1-32: Consider splitting responsibilities and adding utility methods.The
Writerclass has been extended with new functionality for handling references and generating imports. While these additions fit well with the existing structure, they increase the class's responsibilities. Consider the following suggestions:
To adhere more closely to the Single Responsibility Principle, you might want to extract the reference and import handling into a separate class, e.g.,
ReferenceManager.Add utility methods for querying and manipulating the references array, such as
hasReference(reference: Reference): booleanorremoveReference(reference: Reference): void.Here's a sketch of how you might refactor this:
class ReferenceManager { private references: Reference[] = []; public addReference(reference: Reference): void { // Implementation } public generateImports(): string { // Implementation } // Other utility methods } class Writer extends AbstractWriter { private referenceManager = new ReferenceManager(); public addReference(reference: Reference): void { this.referenceManager.addReference(reference); } public toString(skipImports = false): string { const imports = skipImports ? '' : this.referenceManager.generateImports(); return `${imports}\n\n${this.buffer}`; } // Other methods }This refactoring would improve separation of concerns and make the code more modular and easier to maintain.
generators/typescript/codegen/src/ast/__test__/Namespace.test.ts (4)
3-3: Consider removing unused declarationThe
SimpleNamespacedeclaration appears to be unused in the test cases. Consider removing it to keep the test file clean and focused on the actual test cases.
22-52: LGTM: Comprehensive interface tests with a suggestionThese test cases effectively cover scenarios with single and multiple interfaces within namespaces. The use of
ts.Typefor property types enhances type safety.Consider adding a test case for an interface with multiple properties to ensure correct handling of more complex interface structures.
94-131: LGTM: Excellent complex nested structure test with a suggestionThis test case comprehensively covers a complex scenario with multiple levels of nesting, exported namespaces, and interfaces at different levels. It provides thorough coverage for verifying the correct handling of complex namespace structures.
Consider adding assertions for individual elements (e.g., checking if specific interfaces or nested namespaces exist) in addition to the snapshot test. This could provide more granular feedback if the test fails.
1-133: Excellent test coverage with a minor suggestionThis test file provides comprehensive coverage of namespace functionality, including simple cases, exported namespaces, interfaces, and complex nested structures. The progressive increase in test complexity is well-designed.
To further enhance the test suite:
- Consider adding error case tests, such as attempting to create invalid namespace structures or adding duplicate interfaces.
- Add a test for namespace merging if it's a supported feature in your implementation.
Overall, this is a robust set of tests that will help ensure the reliability of the namespace generation functionality.
generators/typescript/codegen/src/ast/Namespace.ts (3)
8-11: Correct the comment for clarity.In the
Namespace.Argsinterface, the comment/* Namespace of the name */should be corrected for clarity. It would be clearer as/* Name of the namespace */.Apply this diff to correct the comment:
- /* Namespace of the name */ + /* Name of the namespace */
25-27: Rename parameter for clarity inaddNamespacemethod.The parameter
interface_in theaddNamespacemethod refers to aNamespaceobject. To improve readability and avoid confusion with interfaces, consider renaming the parameter tonamespace_.Apply this diff to rename the parameter:
- public addNamespace(interface_: Namespace): void { - this.elements.push(interface_); + public addNamespace(namespace_: Namespace): void { + this.elements.push(namespace_);
49-52: Review extra blank lines in thewritemethod.In the
writemethod's loop, there are two consecutivewriter.writeLine();calls after each element. This results in extra blank lines in the generated code. If additional spacing is intentional for readability, it's acceptable. Otherwise, consider removing one of thewriteLine();calls to reduce unnecessary whitespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
generators/typescript/codegen/src/ast/__test__/__snapshots__/Namespace.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
- generators/typescript/codegen/src/ast/Namespace.ts (1 hunks)
- generators/typescript/codegen/src/ast/test/Namespace.test.ts (1 hunks)
- generators/typescript/codegen/src/ast/core/AstNode.ts (1 hunks)
- generators/typescript/codegen/src/ast/core/Writer.ts (1 hunks)
- generators/typescript/codegen/src/ast/index.ts (1 hunks)
- generators/typescript/codegen/src/typescript.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- generators/typescript/codegen/src/ast/core/AstNode.ts
- generators/typescript/codegen/src/ast/index.ts
🧰 Additional context used
🔇 Additional comments (6)
generators/typescript/codegen/src/typescript.ts (2)
15-17: LGTM! Well-implemented namespace factory function.The
namespacefunction is correctly implemented and follows the established pattern in the file. It provides a clean way to createAST.Namespaceinstances.
21-21: LGTM! Verify the impact of exposing Reference.The export of
Referencefrom "./ast" is correctly implemented and consistent with the file's export patterns.Please ensure that exposing
Referenceis intentional and doesn't unintentionally expose internal implementation details. Run the following script to check its usage across the codebase:generators/typescript/codegen/src/ast/core/Writer.ts (1)
14-16: LGTM! ThesetAmbientmethod is well-implemented.The
setAmbientmethod correctly sets theisAmbientproperty. It provides a clear way to modify the ambient context of the writer.generators/typescript/codegen/src/ast/__test__/Namespace.test.ts (3)
5-20: LGTM: Well-structured basic test casesThe first two test cases effectively cover the creation of simple and exported namespaces. The use of snapshot testing is appropriate for verifying the generated code structure. These tests provide a solid foundation for more complex scenarios.
54-69: LGTM: Well-structured nested namespace testThis test case effectively demonstrates the creation and nesting of namespaces, including the addition of an interface within the inner namespace. It provides good coverage for verifying the correct structure of nested namespaces.
71-92: LGTM: Comprehensive mixed content namespace testThis test case effectively covers a more complex scenario with both interfaces and nested namespaces within a single namespace. It provides good coverage for verifying the correct handling of mixed content structures.
| public toString(skipImports = false): string { | ||
| const imports: string[] = []; | ||
| if (!skipImports) { | ||
| for (const reference of this.references) { | ||
| switch (reference.args.type) { | ||
| case "module": | ||
| imports.push(`import * as ${reference.args.module} from "${reference.args.source}";`); | ||
| break; | ||
| case "named": | ||
| imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return `${imports.join("\n")}\n\n${this.buffer}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential duplicate imports and handle unknown reference types.
The toString method now correctly generates import statements based on the references array. However, there are two areas for improvement:
- As mentioned in a previous review comment, the method doesn't deduplicate import statements, which could lead to redundant imports.
- The switch statement doesn't handle unknown reference types, which could lead to silent failures if new types are introduced in the future.
Consider implementing the following changes:
- Deduplicate imports:
const importSet = new Set<string>();
// ... in the loop:
importSet.add(`import ...`);
// ... after the loop:
const imports = Array.from(importSet);- Handle unknown reference types:
switch (reference.args.type) {
case "module":
// ... existing code ...
case "named":
// ... existing code ...
default:
console.warn(`Unknown reference type: ${reference.args.type}`);
break;
}These changes will improve the robustness and efficiency of the import generation process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
generators/typescript/codegen/src/ast/index.ts (1)
5-8: LGTM! Consider grouping related imports.The new exports (Interface, Reference, Namespace, and TypescriptFile) are correctly added and consistent with the existing export style. These additions align well with the PR objectives of supporting inlined types and initializing an AST.
As a minor suggestion for improved readability:
Consider grouping related imports together. For example:
export * from "./core"; export { CodeBlock } from "./CodeBlock"; export { Type } from "./Type"; export { Variable } from "./Variable"; export { Interface } from "./Interface"; export { Reference } from "./Reference"; export { Namespace } from "./Namespace"; export { TypescriptFile } from "./TypescriptFile";This organization can make it easier to understand the structure and relationships between the exported entities.
generators/typescript/codegen/src/ast/TypescriptFile.ts (1)
22-29: Consider optimizing whitespace in the write method.The
writemethod adds three newlines after each element, which might lead to excessive whitespace in the output file. Consider reducing this to one or two newlines for better readability.Here's a suggested improvement:
public write(writer: Writer): void { for (const element of this.elements) { writer.writeNode(element); - writer.writeLine(); - writer.writeLine(); writer.writeLine(); } }generators/typescript/codegen/src/ast/__test__/TypescriptFile.test.ts (5)
1-1: Consider making the import statement more explicit.The current import statement
import { ts } from "../..";is concise but may not be immediately clear to other developers. Consider making it more explicit by specifying the exact module or file being imported.For example:
import { ts } from "../../typescript-codegen";or
import { ts } from "../../ast-utils";Replace with the actual module name for better clarity and maintainability.
10-19: LGTM: Good test case for a single interface.This test effectively demonstrates how to add a single interface to a TypescriptFile. The use of
ts.Type.string()for type definition is clear and concise.Consider adding a test case with multiple properties of different types to ensure the
interface_function handles various scenarios correctly.
38-49: LGTM: Good test for namespace functionality.This test case effectively demonstrates how to create a namespace and add an interface to it. It's a good representation of nested structures in TypeScript.
Consider adding a test case with multiple nested levels (e.g., namespace within a namespace) to ensure the
toStringFormatted()method handles deep nesting correctly.
72-95: LGTM: Excellent test for mixed content scenarios.This test case effectively demonstrates the ability to handle mixed content types (top-level interfaces and namespaces) in a TypescriptFile. It's particularly good that it tests adding elements in a non-sequential order, which implicitly checks if the order is preserved in the output.
Consider adding an explicit assertion to verify the order of elements in the generated output, in addition to the snapshot test. This could provide more specific feedback if the order is not maintained correctly in future changes.
1-106: Overall: Excellent test suite with comprehensive coverage.This test file provides a robust set of test cases for the TypescriptFile functionality. It covers a wide range of scenarios from simple to complex, including empty files, single and multiple interfaces, namespaces, and mixed content. The use of snapshot testing is consistent and appropriate for this type of string output testing.
Suggestions for further improvement:
- Consider adding test cases for edge cases, such as very large files or files with deeply nested structures.
- It might be beneficial to add some negative test cases, e.g., trying to add invalid content to a TypescriptFile.
- Consider adding performance tests if the
toStringFormatted()method is expected to handle large files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
generators/typescript/codegen/src/ast/__test__/__snapshots__/TypescriptFile.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
- generators/typescript/codegen/package.json (1 hunks)
- generators/typescript/codegen/src/ast/TypescriptFile.ts (1 hunks)
- generators/typescript/codegen/src/ast/test/TypescriptFile.test.ts (1 hunks)
- generators/typescript/codegen/src/ast/index.ts (1 hunks)
- generators/typescript/codegen/src/typescript.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- generators/typescript/codegen/package.json
🧰 Additional context used
🔇 Additional comments (12)
generators/typescript/codegen/src/typescript.ts (4)
11-13: LGTM: Newinterface_function looks good.The
interface_function is implemented correctly, following the existing pattern in the file. The underscore in the name is a good choice to avoid conflicts with theinterfacekeyword.
15-17: LGTM: Newnamespacefunction is implemented correctly.The
namespacefunction follows the established pattern in the file and is implemented correctly.
25-25: LGTM: New export forReferenceadded.The export of
Referencefrom the "./ast" module has been added correctly. This makes theReferencetype or class available to consumers of this module, which is likely related to the new AST functionality.To verify the usage and necessity of the
Referenceexport, you can run the following script:#!/bin/bash # Description: Check for usages of the `Reference` type or class # Search for usages of Reference in TypeScript files echo "Usages of 'Reference' in TypeScript files:" rg --type typescript "Reference" -C 3 # Search for imports of Reference from this module echo "\nImports of Reference from this module:" rg --type typescript "import.*Reference.*from ['\"]@fern-api/typescript-codegen['\"]" -C 3This will help confirm if the
Referenceexport is being used in the codebase and if it's necessary for external consumers.
19-21: LGTM: Newfilefunction looks good, but verify the no-argument design.The
filefunction is implemented correctly. However, unlike other functions in this file, it doesn't take any arguments. This might be intentional, but it's worth verifying if this aligns with the intended usage and if there are any scenarios where arguments might be needed for file creation.To verify the intended usage of the
filefunction, you can run the following script:This will help confirm if the no-argument design is consistent with how it's used throughout the codebase.
✅ Verification successful
<!-- [monologue] [task verify_review_comment_2/2] Analyzing the shell script output to confirm the `file` function's no-argument design. [observations] - The shell script successfully found usages of the `file` function in various test files. - No additional usages of the `TypescriptFile` constructor were found outside the reviewed function. - This indicates that the `file` function is consistently used without arguments, aligning with its current implementation. [analysis] - Since the `TypescriptFile` constructor is only used within the `file` function and there are no external usages requiring arguments, the no-argument design is appropriate. - The absence of other constructor usages suggests that the function's design is intentional and does not introduce potential issues. [actions] - Conclude that the `file` function's no-argument design is correct and the review comment can be approved. --> **LGTM: The `file` function's no-argument design is confirmed to be appropriate.**🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of the `file` function and `TypescriptFile` constructor # Search for usages of the `file` function echo "Usages of the 'file' function:" rg --type typescript "file\(\)" -C 3 # Search for direct usages of the TypescriptFile constructor echo "\nDirect usages of TypescriptFile constructor:" rg --type typescript "new TypescriptFile" -C 3Length of output: 396
Script:
#!/bin/bash # Description: Check for usages of the `file` function and `TypescriptFile` constructor # Search for usages of the `file` function in TypeScript files echo "Usages of the 'file' function:" rg "file\(\)" --glob "*.ts" --glob "*.tsx" -C 3 # Search for direct usages of the TypescriptFile constructor in TypeScript files echo "\nDirect usages of TypescriptFile constructor:" rg "new TypescriptFile" --glob "*.ts" --glob "*.tsx" -C 3Length of output: 17037
generators/typescript/codegen/src/ast/TypescriptFile.ts (4)
1-4: LGTM: Import statements are appropriate.The import statements are relevant to the file's purpose, importing necessary components for AST manipulation, file system utilities, and related classes.
8-30: LGTM: Well-structured TypescriptFile class.The
TypescriptFileclass is well-structured for managing TypeScript file contents. It extendsAstNode, maintains an ordered list of elements, and provides methods for adding namespaces and interfaces. The use of aWriterin thewritemethod demonstrates good separation of concerns.
1-30: Overall, well-implemented TypescriptFile class with minor suggestions.The
TypescriptFileclass is well-structured and appropriate for managing TypeScript file contents. It provides a clean API for adding namespaces and interfaces, and uses aWriterfor output. Consider the following points:
- Clarify the purpose of the empty
TypescriptFilenamespace.- Optimize the whitespace generation in the
writemethod.These minor improvements will enhance the clarity and efficiency of the code.
6-6: Clarify the purpose of the empty namespace.The
TypescriptFilenamespace is declared but empty. Could you please clarify its intended purpose? If it's a placeholder for future implementations, consider adding a TODO comment. If it's used for type augmentation, it might be helpful to add a comment explaining its usage.To check if this namespace is used elsewhere in the codebase, run the following script:
generators/typescript/codegen/src/ast/__test__/TypescriptFile.test.ts (4)
5-8: LGTM: Well-structured test for an empty TypeScript file.This test case effectively checks the behavior of
toStringFormatted()for an empty TypeScript file. The use of snapshot testing is appropriate for this scenario.
21-36: LGTM: Comprehensive test for multiple interfaces.This test case effectively demonstrates the ability to add multiple interfaces to a TypescriptFile. It's good to see different property types being used (
stringandnumber), which helps ensure type variety is handled correctly.
51-70: LGTM: Comprehensive test for multiple namespaces.This test case effectively demonstrates the ability to add multiple namespaces to a TypescriptFile, each with its own interface. It's good to see that interfaces with the same name (
Interface1andInterface2) can exist in different namespaces without conflict.
97-104: LGTM: Crucial test for element ordering.This test case is excellent as it explicitly verifies that the order of added elements (interfaces and namespaces) is maintained in the generated TypeScript file. This is a critical aspect of the
TypescriptFilefunctionality and it's great to see it being tested directly.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (16)
seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/CreateMovieRequest.ts (1)
6-7: Approved: Improved TypeScript syntaxThe removal of quotation marks around property names in the
CreateMovieRequestinterface is a positive change. This update aligns with modern TypeScript best practices and improves code readability. The types remain appropriate for the properties:stringfortitleandnumberforrating.Consider adding JSDoc comments to provide more context for each property, especially if there are any constraints on the
rating(e.g., a specific range of values).generators/typescript/codegen/src/custom-config/BaseTypescriptCustomConfigSchema.ts (3)
3-6: LGTM: Well-structured schema definitionThe schema definition is correct and well-structured. It properly defines two optional properties using Zod's syntax.
For consistency with TypeScript naming conventions, consider using PascalCase for the constant name:
-const BaseTypescriptCustomConfigSchema = z.object({ +const BaseTypeScriptCustomConfigSchema = z.object({ noSerdeLayer: z.optional(z.boolean()), namespaceExport: z.optional(z.string()) });
8-8: LGTM: Correct type export using Zod inferenceThe type export is correctly defined using
z.infer, ensuring type safety and consistency with the schema definition.If you apply the previous suggestion to use PascalCase for the constant name, make sure to update the type export as well for consistency:
-export type BaseTypescriptCustomConfigSchema = z.infer<typeof BaseTypescriptCustomConfigSchema>; +export type BaseTypeScriptCustomConfigSchema = z.infer<typeof BaseTypeScriptCustomConfigSchema>;
1-8: Great addition: Well-structured custom configuration schemaThis new file successfully introduces a schema for custom TypeScript configuration using Zod, which aligns well with the PR objective of enhancing TypeScript support. The code is concise, well-structured, and follows best practices for schema definition and type inference.
The
BaseTypescriptCustomConfigSchemaprovides a flexible configuration structure with optional properties fornoSerdeLayerandnamespaceExport, which can be useful for customizing TypeScript code generation behavior.Overall, this addition enhances the project's capability to handle custom configurations in a type-safe manner, contributing to the goal of improved TypeScript support.
As the project evolves, consider the following:
- Document the purpose and usage of each configuration option, either through comments or in separate documentation.
- If more configuration options are added in the future, consider grouping related options into nested objects for better organization.
generators/typescript/utils/contexts/tsconfig.json (1)
6-10: LGTM with a minor suggestion.The changes to the
referencesarray look good:
- The addition of the "../../codegen" reference aligns with the PR objectives of supporting inlined types and initializing an AST.
- The new formatting improves readability.
However, there's a minor issue to address:
Consider removing the trailing comma after
{ "path": "../commons" }to ensure compatibility with all JSON parsers. While valid in modern JavaScript, some JSON parsers might have issues with trailing commas.Here's the suggested change:
"references": [ { "path": "../../../../packages/cli/logger" }, - { "path": "../commons", }, + { "path": "../commons" }, { "path": "../../codegen"} ]generators/typescript/codegen/src/ast/AstNode.ts (2)
6-13: LGTM: Well-implemented toString() method with a minor suggestion.The
toString()method effectively uses theWriterclass to generate the string representation of the node. The delegation to awrite()method (presumably implemented by subclasses) is a good design choice.Consider adding a
@throwsor@abstractJSDoc comment for thewrite()method in this class to clarify that subclasses must implement it.
15-17: LGTM: Good use of prettier for formatted output, with a suggestion for error handling.The
toStringFormatted()method effectively uses prettier to format the output, which is excellent for maintaining consistent code style. The use of the 'typescript' parser is appropriate for this context.Consider adding error handling to manage potential exceptions from
prettier.format(). Here's a suggested implementation:public toStringFormatted(): string { try { return prettier.format(this.toString(), { parser: "typescript" }); } catch (error) { console.error("Error formatting AST node:", error); return this.toString(); // Fallback to unformatted string } }This change would make the method more robust and prevent potential crashes due to formatting errors.
generators/typescript/utils/contexts/src/base-context/BaseContext.ts (1)
13-13: LGTM: New V2 property added to BaseContext interface.The new
V2property of typeBaseTypescriptGeneratorContext<BaseTypescriptCustomConfigSchema>has been correctly added to theBaseContextinterface. This addition enhances the functionality of the interface and aligns with the PR objective of supporting inlined types.Consider adding a brief comment explaining the purpose and usage of the
V2property, especially if it represents a new version or iteration of an existing context. This would improve code readability and maintainability.generators/typescript/codegen/src/ast/Variable.ts (1)
Line range hint
5-33: LGTM! Consider a minor readability improvement.The implementation of the
Variablenamespace and class looks good. It correctly extendsAstNodeand thewritemethod properly uses theArgsinterface.For improved readability, consider using object destructuring in the constructor. Here's a suggested change:
export class Variable extends AstNode { - public constructor(private readonly args: Variable.Args) { + public constructor(private readonly { + export: isExport, + const: isConst, + name, + initializer + }: Variable.Args) { super(); } public write(writer: Writer): void { - if (this.args.export) { + if (isExport) { writer.write("export "); } - if (this.args.const) { + if (isConst) { writer.write("const "); } - writer.write(`${this.args.name} = `); - writer.writeNode(this.args.initializer); + writer.write(`${name} = `); + writer.writeNode(initializer); } }This change would make the code more readable and reduce the repetition of
this.args.generators/typescript/sdk/generator/src/contexts/base/BaseContextImpl.ts (2)
21-21: LGTM: New property added to Init interface.The new
v2Contextproperty is correctly typed and added to theInitinterface. This addition aligns with the changes in theBaseContextImplclass.Consider discussing the naming convention for version-specific properties. While
v2Contextis clear, it might be worth establishing a consistent pattern for future versioning (e.g.,contextV2orcontextVersion2).
30-30: LGTM: BaseContextImpl class updated correctly.The
BaseContextImplclass has been correctly updated with a new public propertyV2and the constructor now includes thev2Contextparameter. The assignment in the constructor is well-placed for clarity.Consider aligning the naming convention between the property
V2and the parameterv2Context. For consistency, you might want to use either camelCase for both (e.g.,v2Contextandv2Context) or PascalCase for both (e.g.,V2ContextandV2Context).Also applies to: 37-40
generators/typescript/express/cli/src/ExpressGeneratorCli.ts (1)
50-50: LGTM! Consider renaming the parameter for clarity.The addition of
generatorExecConfig: configto theExpressGeneratorconstructor is a good enhancement. It allows the generator to access the execution configuration directly, which can lead to more flexible and context-aware code generation.For improved clarity, consider renaming the parameter to avoid potential confusion:
-generatorExecConfig: config, +generatorExecConfig: generatorConfig,This change would make it clearer that
generatorConfigis distinct from theconfigobject used for other configuration settings.generators/typescript/express/generator/src/contexts/ExpressContextImpl.ts (3)
82-83: LGTM! Consider adding documentation for the new property.The addition of the
v2Contextproperty to theInitinterface is consistent with the PR objectives of enhancing TypeScript support. This change allows for the integration of a new version of the TypeScript generator context.Consider adding a brief comment to document the purpose and usage of the
v2Contextproperty. This will help future developers understand its role in the initialization process.
105-105: LGTM! Consider adjusting the property name to follow TypeScript conventions.The addition of the
V2property to theExpressContextImplclass corresponds well with the newv2Contextin theInitinterface. This change supports the integration of the new TypeScript generator context.Consider renaming the
V2property tov2Contextorv2to align with TypeScript naming conventions for properties, which typically use camelCase. This would also make it consistent with the naming in theInitinterface.
108-108: LGTM! Consider grouping related initializations.The constructor has been correctly updated to accept the new
v2Contextparameter and initialize theV2property. This change properly integrates the new TypeScript generator context into theExpressContextImplclass.Consider grouping the initialization of
V2with other related property initializations in the constructor. This could improve code readability and organization. For example, you might want to place it near other context-related initializations if there are any.Also applies to: 142-142
generators/typescript/codegen/src/ast/Writer.ts (1)
22-29: Handle unexpected reference types in the switch statementCurrently, the
switchstatement handles"module"and"named"types. If there's a possibility of encountering otherreference.args.typevalues, consider adding adefaultcase to handle unexpected types, which can aid in debugging and error handling.Example:
break; + default: + throw new Error(`Unsupported reference type: ${reference.args.type}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
generators/typescript/.yarn/releases/yarn-1.22.22.cjsis excluded by!**/.yarn/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
- generators/typescript/codegen/package.json (1 hunks)
- generators/typescript/codegen/src/ast/AstNode.ts (1 hunks)
- generators/typescript/codegen/src/ast/CodeBlock.ts (1 hunks)
- generators/typescript/codegen/src/ast/Interface.ts (1 hunks)
- generators/typescript/codegen/src/ast/Namespace.ts (1 hunks)
- generators/typescript/codegen/src/ast/Reference.ts (1 hunks)
- generators/typescript/codegen/src/ast/Type.ts (3 hunks)
- generators/typescript/codegen/src/ast/TypescriptFile.ts (1 hunks)
- generators/typescript/codegen/src/ast/Variable.ts (1 hunks)
- generators/typescript/codegen/src/ast/Writer.ts (1 hunks)
- generators/typescript/codegen/src/ast/core/index.ts (0 hunks)
- generators/typescript/codegen/src/ast/index.ts (1 hunks)
- generators/typescript/codegen/src/context/BaseTypescriptGeneratorContext.ts (1 hunks)
- generators/typescript/codegen/src/context/TypescriptTypeMapper.ts (1 hunks)
- generators/typescript/codegen/src/custom-config/BaseTypescriptCustomConfigSchema.ts (1 hunks)
- generators/typescript/codegen/src/index.ts (1 hunks)
- generators/typescript/codegen/src/typescript.ts (1 hunks)
- generators/typescript/codegen/tsconfig.json (0 hunks)
- generators/typescript/express/cli/src/ExpressGeneratorCli.ts (1 hunks)
- generators/typescript/express/generator/package.json (1 hunks)
- generators/typescript/express/generator/src/ExpressGenerator.ts (6 hunks)
- generators/typescript/express/generator/src/contexts/ExpressContextImpl.ts (4 hunks)
- generators/typescript/express/generator/tsconfig.json (1 hunks)
- generators/typescript/model/type-generator/package.json (1 hunks)
- generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (2 hunks)
- generators/typescript/model/type-generator/tsconfig.json (1 hunks)
- generators/typescript/sdk/cli/package.json (2 hunks)
- generators/typescript/sdk/cli/tsconfig.json (1 hunks)
- generators/typescript/sdk/generator/src/contexts/SdkContextImpl.ts (3 hunks)
- generators/typescript/sdk/generator/src/contexts/base/BaseContextImpl.ts (3 hunks)
- generators/typescript/utils/contexts/package.json (1 hunks)
- generators/typescript/utils/contexts/src/base-context/BaseContext.ts (1 hunks)
- generators/typescript/utils/contexts/tsconfig.json (1 hunks)
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/Movie.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/CreateMovieRequest.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/Movie.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/version.ts (1 hunks)
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/Movie.ts (1 hunks)
💤 Files with no reviewable changes (2)
- generators/typescript/codegen/src/ast/core/index.ts
- generators/typescript/codegen/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- seed/ts-sdk/imdb/noScripts/src/version.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- generators/typescript/codegen/package.json
- generators/typescript/codegen/src/ast/CodeBlock.ts
- generators/typescript/codegen/src/ast/Interface.ts
- generators/typescript/codegen/src/ast/Namespace.ts
- generators/typescript/codegen/src/ast/Reference.ts
- generators/typescript/codegen/src/ast/TypescriptFile.ts
- generators/typescript/codegen/src/typescript.ts
- generators/typescript/model/type-generator/package.json
- generators/typescript/sdk/cli/package.json
- generators/typescript/utils/contexts/package.json
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/Movie.ts
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/Movie.ts
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/Movie.ts
🧰 Additional context used
🪛 Biome
generators/typescript/codegen/src/ast/Type.ts
[error] 165-165: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 171-171: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 177-177: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 184-184: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 190-190: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 197-197: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 204-204: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 210-210: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 216-216: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 222-222: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 233-233: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (37)
generators/typescript/codegen/src/custom-config/BaseTypescriptCustomConfigSchema.ts (1)
1-1: LGTM: Correct import for Zod libraryThe import statement is correct and imports the necessary
zobject from the Zod library, which is used to define the schema.generators/typescript/model/type-generator/tsconfig.json (1)
8-8: LGTM. Verify the new reference.The addition of the reference to the
codegenmodule aligns with the PR objectives of supporting inlined types and initializing an AST. This change appears to be correct and necessary for the new functionality.To ensure the correctness of this change, please run the following verification script:
This script will verify the existence of the codegen directory and check for its usage in the type-generator source files.
generators/typescript/codegen/src/index.ts (5)
2-2: LGTM: Specific export of BaseTypescriptGeneratorContextThis change aligns with the move towards more specific exports, improving the module's interface clarity.
3-3: LGTM: Specific export of TypescriptTypeMapperThis export follows the same pattern of specific exports, contributing to a cleaner and more explicit module interface.
4-4: LGTM: Export of Writer classThis export introduces the Writer class, which is responsible for managing references and generating import statements. This addition aligns with the PR's objective of enhancing TypeScript support and AST initialization.
5-5: LGTM: Type-only export of BaseTypescriptCustomConfigSchemaThis export introduces a type-only export for BaseTypescriptCustomConfigSchema. The use of the 'type' keyword in the export statement is a good practice in TypeScript, as it ensures that this export is used only for type information and not as a value.
2-5: Summary: Improved module interface with specific exportsThe changes in this file represent a significant improvement in the module's interface. By moving from blanket exports to specific, named exports, the code becomes more explicit about what it's exposing, which can lead to better maintainability and easier usage for consumers of this module. The new exports, particularly the Writer class and BaseTypescriptCustomConfigSchema type, align well with the PR's objectives of enhancing TypeScript support and AST initialization.
These changes contribute to a more robust and type-safe codebase, which is particularly important for a TypeScript code generator.
generators/typescript/codegen/src/ast/index.ts (1)
1-9: LGTM! New exports enhance TypeScript support.The changes look good and align with the PR objectives. The new exports for
AstNode,Writer,Interface,Reference,Namespace, andTypescriptFilesuggest improved support for inlined types and AST initialization.However, I noticed a discrepancy between the AI summary and the visible changes. The summary mentions replacing exports from the 'core' module, but this is not visible in the provided code. This might be due to the diff not showing removed lines.
To ensure these changes don't negatively impact other parts of the codebase, please run the following verification script:
This script will help identify any potential issues with imports in other files that might have been affected by these changes.
generators/typescript/codegen/src/ast/AstNode.ts (2)
1-3: LGTM: Imports are appropriate and well-structured.The imports are correctly defined and align with the class's requirements. The use of a local
Writerclass and the externalprettierlibrary for formatting are good choices for the intended functionality.
5-5: LGTM: Well-structured abstract class declaration.The
AstNodeclass is correctly declared as abstract and extendsAbstractAstNode. This design promotes code reuse and provides a solid foundation for implementing specific AST node types.generators/typescript/utils/contexts/src/base-context/BaseContext.ts (2)
5-5: LGTM: New import statement added correctly.The new import statement for
BaseTypescriptGeneratorContextandBaseTypescriptCustomConfigSchemafrom "@fern-api/typescript-codegen" is correctly formatted and aligns with the existing import style in the file. This import supports the newV2property added to theBaseContextinterface.
Line range hint
1-14: Summary: BaseContext enhanced with V2 property for improved TypeScript support.The changes to this file align well with the PR objectives. The addition of the
V2property to theBaseContextinterface, along with the necessary import, sets the foundation for enhanced TypeScript support, particularly for handling inlined types. This modification suggests a potential upgrade or new version of the context, which could be part of a broader refactoring or feature enhancement.To ensure these changes are consistent with the rest of the codebase, let's verify the usage of
BaseContextand the newV2property:This script will help us understand how widely
BaseContextis used, if the newV2property is being utilized, and ifBaseTypescriptGeneratorContextis used elsewhere in the codebase. The results will provide insights into the potential impact of these changes and whether further updates might be necessary in other parts of the project.✅ Verification successful
Verification Successful:
BaseContextEnhancements Align with Codebase UsageThe introduction of the
V2property in theBaseContextinterface has been properly integrated and is being utilized across various parts of the codebase. No inconsistencies or issues were found during the verification process.
- The
V2property is consistently used in contexts extendingBaseContext.- No misuse or absence of the
V2property was detected where it is expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of BaseContext and the new V2 property # Test 1: Check for other occurrences of BaseContext echo "Occurrences of BaseContext:" rg --type typescript "BaseContext" # Test 2: Check for usage of the new V2 property echo "\nUsage of V2 property:" rg --type typescript "BaseContext.*V2" # Test 3: Check for other occurrences of BaseTypescriptGeneratorContext echo "\nOccurrences of BaseTypescriptGeneratorContext:" rg --type typescript "BaseTypescriptGeneratorContext"Length of output: 463
Script:
#!/bin/bash # Description: Verify the usage of BaseContext and the new V2 property with corrected file type # Test 1: Check for other occurrences of BaseContext echo "Occurrences of BaseContext:" rg --type ts "BaseContext" # Test 2: Check for usage of the new V2 property echo "\nUsage of V2 property:" rg --type ts "BaseContext.*V2" # Test 3: Check for other occurrences of BaseTypescriptGeneratorContext echo "\nOccurrences of BaseTypescriptGeneratorContext:" rg --type ts "BaseTypescriptGeneratorContext"Length of output: 5961
Script:
#!/bin/bash # Description: Verify the usage of the V2 property in the codebase # Test 1: Search for any instance where V2 is accessed as a property echo "Instances of V2 property access:" rg --type ts "\.V2\b" # Test 2: Use ast-grep to find property accesses to V2 within classes extending BaseContext echo "\nAST-based search for V2 property access within BaseContext extensions:" ast-grep --lang typescript --pattern "_.V2" .Length of output: 5467
generators/typescript/codegen/src/ast/Variable.ts (1)
1-3: LGTM! Consider verifying import consistency.The changes to the import statements look good and potentially improve the project's modularity. The separation of
AstNodeandWriterinto individual files is a positive step.To ensure consistency across the project, please run the following script to check for any mismatched imports:
This will help identify any files that might need updating to match the new import structure.
generators/typescript/express/generator/tsconfig.json (1)
22-24: LGTM! Verify the necessity of new references.The changes look good. The new references to "../../codegen" and "../../../commons" have been added correctly, maintaining proper JSON syntax.
To ensure these changes align with the project structure and dependencies, please run the following verification script:
This script will help confirm that the referenced projects exist and are properly structured.
✅ Verification successful
Verification Successful: New references are valid.
The referenced directories
codegenandcommonsexist and contain validtsconfig.jsonfiles.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the newly referenced TypeScript projects # Test 1: Check if the 'codegen' directory exists echo "Checking for 'codegen' directory..." fd -t d -d 2 'codegen' generators/typescript # Test 2: Check if the 'commons' directory exists echo "Checking for 'commons' directory..." fd -t d -d 3 'commons' generators # Test 3: Verify if these directories contain TypeScript projects (look for tsconfig.json) echo "Verifying TypeScript projects..." fd -t f 'tsconfig.json' generators/typescript/codegen generators/commonsLength of output: 565
generators/typescript/sdk/generator/src/contexts/base/BaseContextImpl.ts (1)
1-1: LGTM: New imports added correctly.The new import statement for
BaseTypescriptCustomConfigSchemaandBaseTypescriptGeneratorContextfrom "@fern-api/typescript-codegen" is correctly added and aligns with the subsequent changes in the file.generators/typescript/express/generator/package.json (2)
30-31: New dependencies addedTwo new dependencies have been added to the package:
@fern-api/generator-commons@fern-api/typescript-codegenBoth are set to
workspace:*, indicating they are local packages within the monorepo. These additions align with the PR objectives of enhancing TypeScript support and initializing an AST.To ensure these new dependencies are properly integrated and don't introduce any conflicts, please run the following verification script:
#!/bin/bash # Description: Verify new dependencies and their usage # Test 1: Check if the new dependencies are available in the workspace echo "Checking for new dependencies in the workspace:" fd -t d -d 1 "generator-commons|typescript-codegen" ../../.. # Test 2: Verify usage of new dependencies in the codebase echo "Checking usage of new dependencies:" rg --type typescript -e "@fern-api/generator-commons" -e "@fern-api/typescript-codegen" ../src
34-34: Updated @fern-fern/ir-sdk versionThe
@fern-fern/ir-sdkdependency has been updated from version53.8.0to53.14.0. This update may introduce new features or bug fixes that support the PR objectives.To ensure this version update doesn't introduce any breaking changes or unexpected behavior, please run the following verification script:
generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (2)
6-6: LGTM: New import for TypeScript AST handling.The addition of this import suggests a shift towards using a more structured approach for TypeScript code generation, which aligns with the PR objectives of supporting inlined types and initializing an AST.
4-4: Verify the necessity of the new import.A new import for
tsfromts-morphhas been added, but it's not visibly used in the changed parts of the file. Please ensure that this import is necessary and used in other unchanged parts of the file. If it's not used, consider removing it to keep the imports clean.To check the usage of this import, run the following script:
#!/bin/bash # Description: Check for usage of ts from ts-morph # Test: Search for usage of ts from ts-morph rg --type typescript '\bts\.' generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.tsIf this script doesn't return any results, consider removing the import.
generators/typescript/codegen/src/ast/Type.ts (5)
2-20: Improved type definitions and importsThe changes to the imports and the
InternalTypedefinition are well-structured and enhance the type safety of the codebase. The expandedInternalTypenow covers a comprehensive set of types, which should improve code clarity and reduce potential type-related errors.
Line range hint
89-153: Well-implementedwritemethodThe updates to the
writemethod in theTypeclass are well-implemented and consistent with the new type definitions. Each case in the switch statement correctly uses thewriterto output the corresponding string representation for the respective type.
229-237: Well-implementedunionfactory methodThe
unionstatic factory method is well-implemented. It correctly handles the edge case of empty variants by returningType.unknown(), which is a good practice to avoid potential issues. The implementation is consistent with other factory methods and provides a clean way to create union types.🧰 Tools
🪛 Biome
[error] 233-233: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
22-83:⚠️ Potential issueRename interfaces to avoid conflict with built-in types
The new interfaces provide clear type definitions for each supported type, which is great. However, some interface names (
Number,Date,Set,Map, andUndefined) conflict with built-in TypeScript types. This can lead to confusion and potential issues.Consider renaming these interfaces to avoid naming conflicts. For example:
Number→NumberTypeDate→DateTypeSet→SetTypeMap→MapTypeUndefined→UndefinedType
164-237:⚠️ Potential issueReplace
thiswith class nameTypein static methodsThe new static factory methods provide a clean API for creating different types, which is great. However, using
thisin static methods can be confusing becausethisrefers to the class constructor, not an instance. It's clearer to use the class name directly.In all static factory methods, replace
new this({...})withnew Type({...}).For example, in the
number()method:public static number(): Type { - return new this({ + return new Type({ type: "number" }); }Apply this change to all static factory methods in the class.
🧰 Tools
🪛 Biome
[error] 165-165: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 171-171: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 177-177: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 184-184: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 190-190: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 197-197: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 204-204: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 210-210: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 216-216: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 222-222: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 233-233: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
generators/typescript/express/generator/src/contexts/ExpressContextImpl.ts (1)
Line range hint
1-238: Overall, the changes look good and align with the PR objectives.The modifications to
ExpressContextImpl.tssuccessfully introduce support for a new version of the TypeScript generator context. The changes are well-integrated into the existing structure and do not disrupt current functionality. The code is consistent and follows good practices.A few minor suggestions have been made to improve documentation and naming conventions, which would enhance code readability and maintainability.
generators/typescript/codegen/src/ast/Writer.ts (1)
32-33: Verify thatthis.bufferis properly initializedThe
toStringmethod usesthis.buffer, which is expected to be defined in theAbstractWritersuperclass. Ensure thatthis.bufferis correctly initialized to avoid potential runtime errors when generating the output string.generators/typescript/codegen/src/context/BaseTypescriptGeneratorContext.ts (3)
9-22: Class definition and constructor are well-structuredThe
BaseTypescriptGeneratorContextclass is correctly extendingAbstractGeneratorContext, and the constructor properly initializes all required properties. The use of generics withCustomConfigensures type safety and flexibility.
31-37: Error handling ingetTypeDeclarationOrThrowis appropriateThe method
getTypeDeclarationOrThrowcorrectly retrieves the type declaration and throws a descriptive error if not found. This ensures that calling methods are aware of missing types immediately.
39-41: Efficient retrieval ingetTypeDeclarationThe
getTypeDeclarationmethod provides a straightforward way to access type declarations from the intermediate representation.generators/typescript/codegen/src/context/TypescriptTypeMapper.ts (1)
66-82: Handling of unrecognized primitive typesIn the
convertPrimitivemethod, the_othercase returnsts.Type.object()for unrecognized primitive types._other: () => ts.Type.object()Review whether returning
ts.Type.object()is appropriate for unrecognized primitive types. If certain primitives are unsupported, you might want to handle them explicitly or throw an error to prevent unexpected behavior.generators/typescript/sdk/generator/src/contexts/SdkContextImpl.ts (2)
3-3: Addition of necessary imports is appropriateThe new imports from
@fern-api/typescript-codegenare required for the added functionality.
226-231: Ensure thenoSerdeLayerconfiguration is correctThe
noSerdeLayerparameter is set to the negation ofincludeSerdeLayer(noSerdeLayer: !this.includeSerdeLayer). Verify that this inversion is intentional and aligns with the desired configuration for the serialization/deserialization layer.Run the following script to check the usage of
noSerdeLayerassignments in the codebase and confirm consistency:✅ Verification successful
Configuration Verified Successfully
The
noSerdeLayerparameter is correctly set as the negation ofincludeSerdeLayerinSdkContextImpl.ts, ensuring the serialization/deserialization layer configuration aligns with the intended design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'noSerdeLayer' is consistently set based on 'includeSerdeLayer'. # Test: Search for assignments of 'noSerdeLayer' and check for consistent usage. rg --type=ts --no-heading --line-number 'noSerdeLayer:\s*!?this\.includeSerdeLayer'Length of output: 250
generators/typescript/express/generator/src/ExpressGenerator.ts (5)
113-114: Confirm the necessity and usage of v2ContextThe addition of
v2Contextas a private property introduces a new context withinExpressGenerator. Ensure thatv2Contextis appropriately utilized throughout the class and that its introduction serves a clear purpose.
128-134: Ensure correct initialization of BaseTypescriptGeneratorContextThe initialization of
v2ContextwithBaseTypescriptGeneratorContextincludesgeneratorExecConfigand a new instance ofGeneratorNotificationService. Verify thatgeneratorExecConfigcontains the required configuration and that theGeneratorNotificationServiceis correctly instantiated with the appropriate environment.
49-49:⚠️ Potential issueEnsure backward compatibility when modifying the Init interface
Adding
generatorExecConfigto theExpressGenerator.Initinterface may break existing implementations that use this interface. Please verify that all usages ofExpressGenerator.Initare updated to include the newgeneratorExecConfigproperty to maintain compatibility.Run the following script to find all instances of
ExpressGenerator.Initand check if they includegeneratorExecConfig:#!/bin/bash # Description: Verify that all implementations of ExpressGenerator.Init include generatorExecConfig rg "ExpressGenerator\.Init" --type ts -A 5 | rg -L "generatorExecConfig"
115-122:⚠️ Potential issueUpdate constructor parameters and ensure consistency
The constructor now includes
generatorExecConfig. This change requires all instantiations ofExpressGeneratorto be updated with the new parameter. Please verify that all instances whereExpressGeneratoris instantiated have been modified accordingly.Run the following script to locate all instantiations of
ExpressGeneratorand check for the inclusion ofgeneratorExecConfig:#!/bin/bash # Description: Find all instantiations of ExpressGenerator and check for generatorExecConfig rg "new ExpressGenerator\(" --type ts -A 5 | rg -L "generatorExecConfig"
500-500:⚠️ Potential issueVerify the update to ExpressContextImpl to accept v2Context
Passing
v2ContexttoExpressContextImplrequires that its constructor accepts this new parameter. Confirm thatExpressContextImplhas been updated to handlev2Contextand that all usages ofExpressContextImplare consistent with this change.Run the following script to check the constructor and usages of
ExpressContextImpl:
| @@ -9,6 +9,7 @@ | |||
| { "path": "../../utils/abstract-generator-cli" }, | |||
| { "path": "../../utils/commons" }, | |||
| { "path": "../../utils/contexts" }, | |||
| { "path": "../../codegen" }, | |||
There was a problem hiding this comment.
💡 Codebase verification
Warning: The referenced codegen directory does not exist.
Please ensure that the ../../codegen directory is present and contains the necessary files for the CLI to function correctly.
🔗 Analysis chain
LGTM! Verify the referenced directory exists.
The addition of the reference path { "path": "../../codegen" } is appropriate and aligns with the PR objectives of enhancing TypeScript support and initializing an AST. This change suggests that the CLI now depends on or utilizes code from the "codegen" directory.
To ensure the referenced directory exists, please run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the codegen directory
if [ -d "$(dirname "$0")/../../codegen" ]; then
echo "The codegen directory exists."
else
echo "Warning: The codegen directory does not exist."
fi
Length of output: 181
| const file = TypeScriptAST.file(); | ||
|
|
||
| return propertyNode; | ||
| }) | ||
| ], | ||
| isExported: true | ||
| const interface_ = TypeScriptAST.interface_({ | ||
| name: this.typeName, | ||
| export: true, | ||
| properties: this.shape.properties.map((property) => { | ||
| return { | ||
| name: `${this.getPropertyKeyFromProperty(property)}`, | ||
| type: context.V2.tsTypeMapper.convert({ reference: property.valueType }), | ||
| questionMark: | ||
| !this.noOptionalProperties && | ||
| property.valueType.type === "container" && | ||
| property.valueType.container.type === "optional" | ||
| // docs: property.docs != null ? [{ description: property.docs }] : undefined | ||
| }; | ||
| }) | ||
| // extends: this.shape.extends.map((extend) => context.V2.tsTypeMapper.convert({ reference: extend.typeId })) | ||
| }); | ||
|
|
||
| maybeAddDocs(interfaceNode, this.getDocs(context)); | ||
| // maybeAddDocs(interfaceNode, this.getDocs(context)); | ||
|
|
||
| for (const extension of this.shape.extends) { | ||
| interfaceNode.addExtends(getTextOfTsNode(context.type.getReferenceToNamedType(extension).getTypeNode())); | ||
| } | ||
| // for (const extension of this.shape.extends) { | ||
| // interfaceNode.addExtends(getTextOfTsNode(context.type.getReferenceToNamedType(extension).getTypeNode())); | ||
| // } | ||
|
|
||
| file.addInterface(interface_); | ||
|
|
||
| context.sourceFile.set({ | ||
| statements: file.toStringFormatted() | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Commented-out code detected in writeToFile implementation.
The verification script found commented-out code related to adding documentation and extending types, as initially mentioned. This suggests that some functionality might still be missing or intentionally disabled.
Please confirm if these commented sections are intentional. If they are meant to be removed, consider deleting them to clean up the codebase. If they are placeholders for future implementations, ensure they do not hinder the current functionality.
🔗 Analysis chain
Significant improvement in writeToFile implementation.
The new implementation addresses the previous placeholder issue and aligns with the PR objectives. It effectively uses TypeScriptAST to generate the interface, which is a step towards better AST handling. The optional property logic (lines 24-27) looks correct.
However, there are a few points to consider:
-
The commented-out code for adding documentation (line 34) and extending types (lines 36-38) suggests that these features might be missing in the current implementation. Is this intentional?
-
The
extendsproperty in the interface creation (line 31) is also commented out. If inheritance is a required feature, this should be addressed.
To ensure no functionality has been lost, please run the following script:
If this script returns results, please review them to ensure no critical functionality is missing.
Action Required: Re-attempt verification with corrected shell scripts.
Please run the following scripts to verify the presence of TODOs or commented code:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any TODOs or commented code that might indicate missing functionality
# Test: Search for TODO comments and commented code
rg --type typescript '(TODO|//\s*\w)' generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts
Length of output: 342
Script:
#!/bin/bash
# Description: Locate the GeneratedObjectTypeImpl.ts file and search for TODOs or commented code
# Step 1: Find the correct path of GeneratedObjectTypeImpl.ts
fd "GeneratedObjectTypeImpl\.ts$" > found_files.txt
# Step 2: If the file is found, search for TODOs or commented code
if [ -s found_files.txt ]; then
while IFS= read -r file; do
rg '(TODO|//\s*\w)' "$file"
done < found_files.txt
else
echo "GeneratedObjectTypeImpl.ts not found in the repository."
fi
Length of output: 676
| imports.push(`import * as ${reference.args.module} from "${reference.args.source}";`); | ||
| break; | ||
| case "named": | ||
| imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`); |
There was a problem hiding this comment.
Add missing space in the import statement template
There's a missing space between the closing brace and the from keyword in the import statement for named imports. This may lead to syntax errors in the generated code.
Apply this diff to fix the issue:
-imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`);
+imports.push(`import { ${reference.args.name} } from "${reference.args.source}";`);📝 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.
| imports.push(`import { ${reference.args.name}} from "${reference.args.source}";`); | |
| imports.push(`import { ${reference.args.name} } from "${reference.args.source}";`); |
| import { Reference } from "./Reference"; | ||
|
|
||
| export class Writer extends AbstractWriter { | ||
| /* Import statements */ |
There was a problem hiding this comment.
Remove or relocate the misplaced comment inside the class
The comment /* Import statements */ is placed inside the class body but refers to import statements at the top of the file. This might cause confusion. Consider removing it or moving it above the import statements.
Apply this diff to remove the comment:
- /* Import statements */Alternatively, move the comment above the import statements:
+/* Import statements */
import { AbstractWriter } from "@fern-api/generator-commons";
import { Reference } from "./Reference";📝 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.
| /* Import statements */ |
| public getNamespaceExport(): string { | ||
| if (this.customConfig.namespaceExport != null) { | ||
| return this.customConfig.namespaceExport; | ||
| } | ||
| return `${upperFirst(camelCase(this.config.organization))}${upperFirst(camelCase(this.config.workspaceName))}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a separator between organization and workspace names
In the getNamespaceExport method, the namespace is constructed by concatenating the organization and workspace names without a separator. This could lead to unintentionally merged names or readability issues, especially if the names are similar or contain the same characters at the junction.
Apply this diff to include an underscore between the names for clarity:
return this.customConfig.namespaceExport;
}
-return `${upperFirst(camelCase(this.config.organization))}${upperFirst(camelCase(this.config.workspaceName))}`;
+return `${upperFirst(camelCase(this.config.organization))}_${upperFirst(camelCase(this.config.workspaceName))}`;📝 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.
| public getNamespaceExport(): string { | |
| if (this.customConfig.namespaceExport != null) { | |
| return this.customConfig.namespaceExport; | |
| } | |
| return `${upperFirst(camelCase(this.config.organization))}${upperFirst(camelCase(this.config.workspaceName))}`; | |
| } | |
| public getNamespaceExport(): string { | |
| if (this.customConfig.namespaceExport != null) { | |
| return this.customConfig.namespaceExport; | |
| } | |
| return `${upperFirst(camelCase(this.config.organization))}_${upperFirst(camelCase(this.config.workspaceName))}`; | |
| } |
| if (value.internalType.type === "object") { | ||
| // object map values should be nullable. | ||
| return ts.Type.record(key, ts.Type.union([value, ts.Type.undefined()])); | ||
| } | ||
| return Type.record(key, value); |
There was a problem hiding this comment.
Inconsistent usage of ts.Type and Type
In the convertContainer method, there is inconsistent usage of ts.Type and Type.
-
Line 50:
return ts.Type.record(key, ts.Type.union([value, ts.Type.undefined()]));
-
Line 52:
return Type.record(key, value);
This inconsistency may lead to confusion or errors if ts.Type and Type are different objects.
Consider using a consistent reference throughout the method. For example, use ts.Type.record in both cases:
- return Type.record(key, value);
+ return ts.Type.record(key, value);| private convertContainer({ container }: { container: ContainerType }): Type { | ||
| switch (container.type) { | ||
| case "list": | ||
| return ts.Type.array(this.convert({ reference: container.list })); |
There was a problem hiding this comment.
Consistent usage of ts.Type.array
In the convertContainer method, there is inconsistent usage of ts.Type.array and Type.array.
-
Line 44:
return ts.Type.array(this.convert({ reference: container.list }));
-
Line 55:
return Type.array(this.convert({ reference: container.set }));
To maintain consistency, consider using ts.Type.array in both instances:
- return Type.array(this.convert({ reference: container.set }));
+ return ts.Type.array(this.convert({ reference: container.set }));Also applies to: 55-55
| throw new Error(`Unrecognized literal type ${val}`); | ||
| } |
There was a problem hiding this comment.
Runtime error on unrecognized literal types
In the convertLiteral method, an error is thrown at runtime for unrecognized literal types.
throw new Error(`Unrecognized literal type ${val}`);Throwing a runtime error may not be ideal. Consider handling this at compile time or by returning a ts.Type.never() to represent an impossible type:
- throw new Error(`Unrecognized literal type ${val}`);
+ return ts.Type.never();This approach helps in type safety and avoids potential runtime exceptions.
| @@ -150,6 +151,7 @@ export class SdkContextImpl implements SdkContext { | |||
| public readonly inlineFileProperties: boolean; | |||
| public readonly generateOAuthClients: boolean; | |||
| public readonly omitUndefined: boolean; | |||
| public V2: BaseTypescriptGeneratorContext<BaseTypescriptCustomConfigSchema>; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming V2 to a more descriptive property name
The property name V2 may not clearly convey its purpose or functionality. Using a more descriptive name would enhance code readability and maintainability.
| noOptionalProperties: config.noOptionalProperties, | ||
| respectInlinedTypes: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding configuration values
The respectInlinedTypes option is set to true directly within the TypeGenerator configuration. Consider making this configurable or using the existing configuration mechanisms to allow flexibility.
You could modify the configuration to include respectInlinedTypes as a configurable option:
- respectInlinedTypes: true
+ respectInlinedTypes: config.respectInlinedTypesEnsure that respectInlinedTypes is defined in the ExpressGenerator.Config interface and is appropriately set during initialization.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
generators/typescript/codegen/src/ast/DocString.ts (2)
1-3: Remove unused importThe
CodeBlockimport on line 2 is not used in this file. Consider removing it to keep the imports clean and avoid potential confusion.Apply this diff to remove the unused import:
import { AstNode } from "./AstNode"; -import { CodeBlock } from "./CodeBlock"; import { Writer } from "./Writer";
10-20: Approve with suggestion: Improve multi-line comment formattingThe
writemethod effectively handles both single-line and multi-line doc strings. However, there's a potential improvement for multi-line comments:Currently, if a line in the multi-line comment is empty, it will result in a line with just
*, which includes trailing whitespace. This might not be desirable in some coding standards.Consider trimming each line before adding the
*prefix. Here's a suggested improvement:public write(writer: Writer): void { if (!this.docs.includes("\n")) { writer.writeLine(`/* ${this.docs} */`); } else { writer.writeLine(`/**`); this.docs.split("\n").forEach((line) => { - writer.writeLine(` * ${line}`); + writer.writeLine(line.trim() ? ` * ${line}` : ` *`); }); writer.writeLine(" */"); } }This change will output
*(without trailing space) for empty lines in the comment, which is often preferred in multi-line comment formatting.generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (2)
4-4: Remove unused importThe
tsimport fromts-morphis not used in the visible changes. Consider removing this import to keep the codebase clean.-import { ts } from "ts-morph";
31-32: Review commented-out codeThere's commented-out code for extending types. If this functionality is intended to be implemented, consider creating a separate task or TODO item to track it. If it's no longer needed, it's best to remove it entirely to keep the codebase clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- generators/typescript/codegen/src/ast/AstNode.ts (1 hunks)
- generators/typescript/codegen/src/ast/DocString.ts (1 hunks)
- generators/typescript/codegen/src/ast/Interface.ts (1 hunks)
- generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (2 hunks)
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/Movie.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/CreateMovieRequest.ts (1 hunks)
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/Movie.ts (1 hunks)
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/Movie.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- generators/typescript/codegen/src/ast/AstNode.ts
- seed/ts-sdk/imdb/no-custom-config/src/api/resources/imdb/types/Movie.ts
- seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/Movie.ts
- seed/ts-sdk/imdb/omit-undefined/src/api/resources/imdb/types/Movie.ts
🧰 Additional context used
🔇 Additional comments (8)
seed/ts-sdk/imdb/noScripts/src/api/resources/imdb/types/CreateMovieRequest.ts (1)
6-7: Approved: Improved TypeScript syntax for interface propertiesThe changes to the
CreateMovieRequestinterface properties improve the code by adopting a more idiomatic TypeScript syntax. Removing the quotation marks around property names ("title"totitleand"rating"torating) is a common practice in TypeScript and enhances readability. This change doesn't affect the runtime behavior and aligns with TypeScript best practices.generators/typescript/codegen/src/ast/DocString.ts (1)
5-8: LGTM: Class declaration and constructorThe class declaration and constructor are well-implemented:
- Extending
AstNodeis appropriate for this AST-related class.- Using
private readonlyfor thedocsparameter ensures immutability and encapsulation.- Calling
super()in the constructor is correct when extending a class.generators/typescript/codegen/src/ast/Interface.ts (4)
1-6: LGTM: Imports are well-organized and relevant.The imports are concise, relevant to the file's functionality, and well-organized. No issues found.
7-31: LGTM: Well-structured Interface namespace.The
Interfacenamespace is well-defined with comprehensiveArgsandPropertyinterfaces. The structure accurately represents the components needed for TypeScript interface generation.
26-26: Fix typo in comment as previously suggested.As mentioned in a previous review, there's a typo in the comment for the
questionMarkproperty.Apply this diff to fix the typo:
- /* Quesiton mark */ + /* Question mark */
33-79: LGTM: Well-implemented Interface class with suggested improvements.The
Interfaceclass is well-structured and correctly implements thewritemethod to generate TypeScript interfaces. The handling ofextendshas been implemented as suggested in a previous review.However, there are two minor improvements to consider:
- Remove the unnecessary escape character in the template string:
-writer.write(`"${property.name}\"`); +writer.write(`"${property.name}"`);
- Consider using
writeNodeinstead ofwriteNodeStatementfor property types to avoid adding unnecessary semicolons or newlines:-writer.writeNodeStatement(property.type); +writer.writeNode(property.type); +writer.write(";"); +writer.writeLine();These changes will enhance the code quality and ensure consistent formatting.
generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts (2)
15-39: Improved implementation ofwriteToFilemethodThe new implementation of the
writeToFilemethod aligns well with the PR objectives. It effectively usesTypeScriptASTto generate the interface, which is a step towards better AST handling and support for inlined types. The logic for determining optional properties (lines 24-27) looks correct and comprehensive.
15-39: Verify impact of changes on the codebaseThe
writeToFilemethod has undergone significant changes. It's important to verify that these changes don't negatively impact other parts of the codebase that might depend on the previous implementation.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
generators/typescript/sdk/cli/src/custom-config/SdkCustomConfig.ts (1)
37-37: LGTM! Consider adding documentation for the new property.The addition of
respectInlinedTypesaligns well with the PR objective of supporting inlined types in TypeScript. This boolean flag provides a way to configure the behavior of the SDK generator regarding inlined types.Consider adding a brief comment above this property to explain its purpose and impact on the SDK generation process. This would help maintainers and users understand when and how to use this configuration option.
generators/typescript/sdk/cli/src/custom-config/schema/SdkCustomConfigSchema.ts (1)
41-43: LGTM! Consider adding a comment for the new property.The addition of the
respectInlinedTypesproperty is correct and follows the existing pattern in the schema. It's properly defined as an optional boolean, which is consistent with other similar properties in the schema.Consider adding a brief comment explaining the purpose of the
respectInlinedTypesproperty, similar to how some other properties have comments (e.g., "beta (not in docs)" or "deprecated"). This would improve the maintainability of the code and help other developers understand its usage.generators/typescript/codegen/src/ast/__test__/Writer.test.ts.ts (5)
10-19: LGTM: Good test for single interface generationThis test case effectively covers the scenario of generating a TypeScript file with a single interface. It demonstrates the basic usage of the
Writerclass and thets.interface_utility.Consider adding an assertion to check if the generated output contains the expected interface name and property. This would make the test more robust against potential changes in the snapshot format.
21-36: LGTM: Good test for multiple interface generationThis test case effectively covers the scenario of generating a TypeScript file with multiple interfaces. It demonstrates the ability of the
Writerclass to handle multiple interfaces and different property types.Consider adding a test case that includes an interface with multiple properties to ensure the
Writerclass can handle more complex interface structures.
51-70: LGTM: Good test for multiple namespace generationThis test case effectively covers the scenario of generating a TypeScript file with multiple namespaces, each containing an interface. It demonstrates the ability of the
Writerclass to handle multiple namespaces and different property types within namespaces.Consider adding a test case that includes nested namespaces to ensure the
Writerclass can handle more complex namespace structures.
97-104: LGTM: Good test for element order preservationThis test case effectively covers the scenario of maintaining the correct order of added elements in the generated TypeScript file. It ensures that the
Writerclass preserves the order in which interfaces and namespaces are added.Consider adding assertions to explicitly check the order of elements in the generated output, rather than relying solely on the snapshot. This would make the test more robust and easier to debug if it fails.
1-106: Overall: Comprehensive test suite with room for minor improvementsThis test file provides a thorough set of test cases for the
Writerclass, covering various scenarios from simple to complex. The use of snapshot testing ensures consistency in the generated output across different runs.While the test suite is comprehensive, consider the following improvements:
- Add more specific assertions in addition to snapshot testing to make the tests more robust and easier to debug.
- Include test cases for edge cases, such as empty namespaces or interfaces with no properties.
- Consider adding tests for error handling scenarios, if applicable to the
Writerclass.generators/typescript/sdk/cli/src/SdkGeneratorCli.ts (2)
62-63: LGTM! Consider adding a comment for the new property.The addition of the
respectInlinedTypesproperty is implemented correctly and follows the existing pattern. For consistency with other properties, consider adding a brief comment explaining its purpose.You could add a comment like this:
// Whether to respect inlined types in the generated code respectInlinedTypes: parsed?.respectInlinedTypes ?? false
139-140: LGTM! Consider aligning the property assignment style.The addition of the
respectInlinedTypesproperty to theSdkGeneratorconfiguration is correct and consistent with the implementation inparseCustomConfig. For better readability and consistency with other properties, consider aligning the assignment style.You could update the assignment like this:
useBigInt: customConfig.useBigInt ?? false, respectInlinedTypes: customConfig.respectInlinedTypes ?? falseThis removes the optional chaining operator, making it consistent with other property assignments and relying on the default value set in
parseCustomConfig.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- generators/typescript/codegen/src/ast/AstNode.ts (1 hunks)
- generators/typescript/codegen/src/ast/Writer.ts (1 hunks)
- generators/typescript/codegen/src/ast/test/Writer.test.ts.ts (1 hunks)
- generators/typescript/codegen/src/context/TypescriptTypeMapper.ts (1 hunks)
- generators/typescript/codegen/src/custom-config/BaseTypescriptCustomConfigSchema.ts (1 hunks)
- generators/typescript/sdk/cli/src/SdkGeneratorCli.ts (2 hunks)
- generators/typescript/sdk/cli/src/custom-config/SdkCustomConfig.ts (1 hunks)
- generators/typescript/sdk/cli/src/custom-config/schema/SdkCustomConfigSchema.ts (1 hunks)
- generators/typescript/sdk/generator/src/SdkGenerator.ts (3 hunks)
- generators/typescript/sdk/generator/src/contexts/SdkContextImpl.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- generators/typescript/codegen/src/ast/AstNode.ts
- generators/typescript/codegen/src/context/TypescriptTypeMapper.ts
🧰 Additional context used
🔇 Additional comments (16)
generators/typescript/codegen/src/custom-config/BaseTypescriptCustomConfigSchema.ts (4)
1-1: LGTM: Zod import is correct.The import statement for Zod is correct and necessary for defining the schema.
3-7: LGTM: Schema definition is correct and aligns with PR objective.The
BaseTypescriptCustomConfigSchemais well-defined using Zod. All properties are optional, providing flexibility in configuration. TherespectInlinedTypesproperty aligns with the PR objective of supporting inlined types.
9-9: LGTM: Type export enhances type safety.The exported
BaseTypescriptCustomConfigSchematype, inferred from the Zod schema, allows for type-safe usage of the configuration throughout the codebase. This is a best practice when working with Zod schemas.
1-9: Overall assessment: Well-structured and aligned with PR objective.This new file successfully introduces a schema for custom configuration using Zod, including support for inlined types as per the PR objective. The code is well-structured, follows best practices, and enhances type safety in the codebase.
generators/typescript/sdk/cli/src/custom-config/schema/SdkCustomConfigSchema.ts (1)
43-43: Verify implementation ofrespectInlinedTypesin the SDK generation process.The addition of the
respectInlinedTypesproperty suggests a new feature in how inlined types are handled. To ensure this change is fully implemented:
- Verify that the
SdkGeneratorclass or similar components have been updated to utilize this new configuration option.- Check if any documentation (e.g., README, API docs) needs to be updated to reflect this new feature.
Run the following script to check for the usage of
respectInlinedTypesin the codebase:This will help ensure that the new configuration option is properly implemented and used throughout the codebase.
✅ Verification successful
Implementation of
respectInlinedTypesVerified in SDK Generation Process.The
respectInlinedTypesproperty is properly implemented and utilized across the SDK generation components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of respectInlinedTypes in TypeScript files # Test: Look for respectInlinedTypes in TypeScript files echo "Searching for respectInlinedTypes usage:" rg --type typescript 'respectInlinedTypes' # Test: Check SdkGenerator for potential implementation echo "Checking SdkGenerator implementation:" rg --type typescript 'class SdkGenerator' -A 20Length of output: 338
Script:
#!/bin/bash # Description: Search for usage of respectInlinedTypes in TypeScript files # Test: Look for respectInlinedTypes in .ts and .tsx files echo "Searching for respectInlinedTypes usage:" rg 'respectInlinedTypes' --glob '*.ts' --glob '*.tsx' # Test: Check SdkGenerator for potential implementation in .ts and .tsx files echo "Checking SdkGenerator implementation:" rg 'class SdkGenerator' -A 20 --glob '*.ts' --glob '*.tsx'Length of output: 14113
generators/typescript/codegen/src/ast/__test__/Writer.test.ts.ts (3)
5-8: LGTM: Good base case testThis test case effectively covers the scenario of an empty TypeScript file, which is a good starting point for testing the
Writerclass.
38-49: LGTM: Good test for namespace generationThis test case effectively covers the scenario of generating a TypeScript file with a namespace containing an interface. It demonstrates the ability of the
Writerclass to handle namespaces and nested structures.
72-95: LGTM: Excellent test for mixed content generationThis test case effectively covers a complex scenario of generating a TypeScript file with mixed content, including top-level interfaces and a namespace with a nested interface. It demonstrates the versatility of the
Writerclass in handling various structures within the same file.generators/typescript/sdk/cli/src/SdkGeneratorCli.ts (1)
Line range hint
1-180: Overall, the changes look good and are well-implemented.The addition of the
respectInlinedTypesproperty to both theparseCustomConfigandgenerateTypescriptProjectmethods is consistent with the existing code structure. The implementation follows the established patterns for handling configuration properties.Minor suggestions for improvement:
- Adding a comment for the new property in
parseCustomConfig.- Aligning the property assignment style in
generateTypescriptProject.These changes enhance the maintainability and readability of the code without affecting its functionality.
generators/typescript/sdk/generator/src/contexts/SdkContextImpl.ts (5)
3-3: New import added for BaseTypescriptGeneratorContext and BaseTypescriptCustomConfigSchemaThe addition of these imports suggests that the
SdkContextImplclass is being extended to incorporate functionality from the@fern-api/typescript-codegenpackage. This change aligns with the PR objective of enhancing TypeScript support.
72-72: New property added to Init interfaceThe
parsedConfigproperty of typeSdkGenerator.Confighas been added to theInitinterface. This addition allows for more configuration options to be passed to theSdkContextImplconstructor, which is in line with the PR's goal of supporting inlined types.
Line range hint
1-358: Summary: Enhanced TypeScript support with inlined typesThe changes in this file successfully implement the PR objectives:
- A new
V2property of typeBaseTypescriptGeneratorContexthas been added, enhancing TypeScript support.- The
parsedConfigparameter in the constructor and therespectInlinedTypesoption in theV2initialization directly support inlined types.- These modifications provide a more flexible and powerful TypeScript code generation capability.
The implementation is clean, consistent, and well-integrated with the existing code structure. Great job on enhancing the TypeScript support in the SDK generator!
Line range hint
159-238: Constructor updated to initialize new V2 propertyThe constructor has been modified to:
- Accept a new
parsedConfigparameter.- Initialize the
V2property with a new instance ofBaseTypescriptGeneratorContext.These changes are consistent with the new property additions and support the PR's objective of enhancing TypeScript support. The
V2initialization includes arespectInlinedTypesoption, which directly relates to the PR's goal of supporting inlined types.To ensure the
parsedConfigparameter is used correctly, let's verify its usage within the class:#!/bin/bash # Search for usage of parsedConfig within the class rg --type typescript 'parsedConfig\.' generators/typescript/sdk/generator/src/contexts/SdkContextImpl.ts
156-156: New V2 property added to SdkContextImpl classThe
V2property of typeBaseTypescriptGeneratorContext<BaseTypescriptCustomConfigSchema>has been added to theSdkContextImplclass. This property seems to be part of a new version or extension of the TypeScript generator context.To ensure this new property is used correctly throughout the codebase, let's verify its usage:
✅ Verification successful
To address the issue with the previous search command, let's execute a revised search targeting
.tsfiles directly.
To accurately search for the usage of the
V2property in TypeScript files, let's refine the search command to correctly target.tsfiles within the specified directory.
To ensure the
V2property is properly integrated and utilized within the codebase, let's search for any references toV2beyond its declarations.
To locate any references to the
V2property beyond its declarations, let's use a simplified search pattern that avoids unsupported regex features.
Verification Complete:
V2Property Added CorrectlyThe
V2property has been successfully added to theSdkContextImplandBaseContextImplclasses. There are currently no usages of this property elsewhere in the codebase, indicating that it is ready for future implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the V2 property rg --type typescript 'V2\s*:' generators/typescript/sdk/generator/srcLength of output: 141
Script:
#!/bin/bash # Search for usage of the V2 property in TypeScript files rg 'V2\s*:' 'generators/typescript/sdk/generator/src/**/*.ts'Length of output: 151
Script:
#!/bin/bash # Search for usage of the V2 property in TypeScript files within the target directory rg 'V2\s*:' 'generators/typescript/sdk/generator/src' --glob '*.ts'Length of output: 369
Script:
#!/bin/bash # Search for references to the V2 property excluding its declarations rg 'V2(?=\s*[.:])' 'generators/typescript/sdk/generator/src' --glob '*.ts'Length of output: 293
Script:
#!/bin/bash # Search for references to the V2 property excluding its declarations rg 'V2\s*[.:]' 'generators/typescript/sdk/generator/src' --glob '*.ts'Length of output: 372
generators/typescript/sdk/generator/src/SdkGenerator.ts (2)
128-128: LGTM: New configuration option added.The addition of
respectInlinedTypes: booleanto theConfiginterface aligns well with the PR objective to support inlined types. The property name is clear and descriptive.
1257-1257: LGTM: Parsed config added to SdkContextImpl.The addition of
parsedConfig: this.config,to the SdkContextImpl constructor is a good change. It makes the parsed configuration available in the SdkContextImpl, which is consistent with the overall changes to support new configuration options.
| retainOriginalCasing: config.retainOriginalCasing, | ||
| respectInlinedTypes: true, |
There was a problem hiding this comment.
💡 Codebase verification
Use config.respectInlinedTypes Instead of Hardcoding to true.
The respectInlinedTypes option is currently hardcoded to true in SdkGenerator.ts. For consistency and to allow configurability, it should use the value from the config object.
- File:
generators/typescript/sdk/generator/src/SdkGenerator.ts
🔗 Analysis chain
Verify the hardcoded value for respectInlinedTypes.
The respectInlinedTypes option is set to true directly, instead of using the value from the config object. This might be intentional, but it's worth confirming if it should use config.respectInlinedTypes instead.
To check if this is intentional, you can run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of respectInlinedTypes in the codebase
rg 'respectInlinedTypes'
Length of output: 1914
| /** | ||
| * The ordered elements inside of a namespace | ||
| */ | ||
| private elements: (Interface | Namespace)[] = []; |
There was a problem hiding this comment.
Remove or utilize the unused 'elements' property
The elements property is initialized and items are added via addNamespace and addInterface, but it's never used elsewhere in the class. This suggests that the property may be unnecessary or there's missing logic to utilize it.
Consider removing the elements property if it's not needed, or implement the logic to use it appropriately.
Also applies to: 23-25, 31-33
| public addNamespace(interface_: Namespace): void { | ||
| this.elements.push(interface_); | ||
| } |
There was a problem hiding this comment.
Rename parameter to 'namespace_' in 'addNamespace' method
The addNamespace method expects a parameter of type Namespace, but it's currently named interface_, which may cause confusion. Renaming it to namespace_ will improve clarity and consistency.
Apply this diff to rename the parameter:
- public addNamespace(interface_: Namespace): void {
- this.elements.push(interface_);
+ public addNamespace(namespace_: Namespace): void {
+ this.elements.push(namespace_);
+ }📝 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.
| public addNamespace(interface_: Namespace): void { | |
| this.elements.push(interface_); | |
| } | |
| public addNamespace(namespace_: Namespace): void { | |
| this.elements.push(namespace_); | |
| } |
| public addReference(reference: Reference): void { | ||
| this.references.push(reference); | ||
| } |
There was a problem hiding this comment.
Prevent duplicate imports by deduplicating references
Currently, addReference adds references without checking for duplicates, which may lead to duplicate import statements. Consider modifying addReference to avoid adding duplicate references.
Here's a possible modification:
public addReference(reference: Reference): void {
+ if (!this.references.some((ref) => ref.equals(reference))) {
this.references.push(reference);
+ }
}Ensure that the Reference class implements an equals method for proper comparison.
Committable suggestion was skipped due to low confidence.
| writer.writeLine(); | ||
| } | ||
| } | ||
| } |
generators/typescript/codegen/src/ast/__test__/__snapshots__/TypescriptFile.test.ts.snap
Outdated
Show resolved
Hide resolved
| ...typeDeclaration.name.fernFilepath.packagePath.map((path) => path.pascalCase.safeName), | ||
| typeDeclaration.name.name.pascalCase.safeName | ||
| ], | ||
| source: `${typeDeclaration.name.fernFilepath.allParts.map(() => "../../../").join("")}` |
There was a problem hiding this comment.
Is ../../../ reliable at this point? How do we know if we're always that far nested within?
There was a problem hiding this comment.
Yup -- every fern file path part is 3 directories nested for dumb legacy reasons
Co-authored-by: Alex McKinney <[email protected]>
Co-authored-by: Alex McKinney <[email protected]>
Summary by CodeRabbit
New Features
AstNode,Writer,DocString, andTypescriptFile, enhancing TypeScript code generation capabilities.respectInlinedTypesconfiguration option to allow more customization in the SDK generation process.InternalTypedefinition to include additional types for improved type handling.Bug Fixes
@fern-fern/ir-sdkto ensure compatibility and access to new features.Documentation
Writerclass to validate output consistency across various scenarios.Refactor
SdkGeneratorandSdkContextImplclasses.