fix(typescript): alias imported types in (de)serialization schemas #5085
Conversation
This fixes an issue where a type imported in a (de)serialization schema has
the same name as the exported schema, causing build errors.
Without this fix, we'd have schemas generated looking like so:
```ts
/**
* This file was auto-generated by Fern from our API Definition.
*/
import * as serializers from "../../../index";
import * as Engine from "../../../../api/index";
import * as core from "../../../../core";
import { SecurityFinding } from "../../ocsf/resources/v110/resources/securityfinding/resources/classes/types/SecurityFinding";
export const SecurityFinding: core.serialization.ObjectSchema<serializers.SecurityFinding.Raw, Engine.SecurityFinding> = SecurityFinding;
export declare namespace SecurityFinding {
type Raw = SecurityFinding.Raw;
}
```
The imported `SecurityFinding` type shares the same name as the
serialization schema, which then causes build errors that are hard and
awkward to fix in post processing of the generated source code.
After this fix, the generated code looks like so:
```ts
/**
* This file was auto-generated by Fern from our API Definition.
*/
import * as serializers from "../../../index";
import * as Engine from "../../../../api/index";
import * as core from "../../../../core";
import { SecurityFinding as SecurityFindingType } from "../../ocsf/resources/v110/resources/securityfinding/resources/classes/types/SecurityFinding";
export const SecurityFinding: core.serialization.ObjectSchema<serializers.SecurityFinding.Raw, Engine.SecurityFinding> = SecurityFindingType;
export declare namespace SecurityFinding {
type Raw = SecurityFindingType.Raw;
}
```
The added `Type` suffix ensures the names do not conflict, solving any build
issues that would otherwise arise.
There was a problem hiding this comment.
Author Notes
@dsinghvi we've been stuck on v 0.12.7 of the TS SDK for some time due to this issue. I believe the issue was introduced with #3206, but the changes there seem sound and does improve performance. The changes I'm proposing here does not seem to break anything else in our project, but I have to admit all the indirection in this project made it extremely hard to even pinpoint where the issue is introduced, let alone trying to fix it.
This seems like a safe change to make, but your guidance here is much appreciated.
| @@ -146,7 +146,7 @@ export class TypeSchemaContextImpl implements TypeSchemaContext { | |||
| useDynamicImport: false, | |||
| namespaceImport: "serializers" | |||
| } | |||
| : { type: "direct" }, | |||
| : { type: "direct", alias: `${typeName.name.originalName}Type` }, | |||
There was a problem hiding this comment.
The alias here is identical to the alias on line 178. If they aren't the same type is imported twice, with different aliases. (Or indeed, if no alias is specified with the conflicting name.)
| @@ -175,7 +175,7 @@ export class TypeSchemaContextImpl implements TypeSchemaContext { | |||
| return { type: "fromRoot", useDynamicImport: false, namespaceImport: "serializers" }; | |||
| } else if (isGeneratingSchema) { | |||
| // Return default import strategy or another strategy based on your logic | |||
| return { type: "direct" }; | |||
| return { type: "direct", alias: `${typeName.name.originalName}Type` }; | |||
There was a problem hiding this comment.
See previous comment about line 149, and the aliases being identical. It may be meaningful to extract this into a utility function to make this less prone to breakage. On the other hand, importing the same type twice with different aliases isn't going to break anything, even if it is redundant.
|
The failed preview-docs / run (pull_request) check does not seem to be related to these changes, unless I'm missing something, here's the error message: |
|
The failed seed / typescript-sdk (pull_request) check however does seem related to these changes. Looks like the test snapshots need updating. I've tried running The error message is cut, but not when trying to run tests in which case it says to run: brew install go && go install github.com/google/gnostic/cmd/protoc-gen-openapi@latestI did try this, but to no avail. I'm not quite sure how to update these snapshots now. |
|
@dsinghvi I wonder if maybe you have some insights into how I may update the snapshots? |
|
@dsinghvi do you think there's any value in this change? It fixes a very real issue in production but of course if this is the wrong approach or a won't fix I'd be happy to receive such feedback so we can adjust accordingly. |
@amckinney apologies if you're not the right person to ping, I just saw your comment on #3453 and figured since I've not been able to get a response from @dsinghvi maybe you could help? Is this PR worth pursuing you think, or should we resolve this issue some other way and if so would you be able to provide guidance? |
This fixes an issue where a type imported in a (de)serialization schema has the same name as the exported schema, causing build errors.
Without this fix, we'd have schemas generated looking like so:
The imported
SecurityFindingtype shares the same name as the serialization schema, which then causes build errors that are hard and awkward to fix in post processing of the generated source code.After this fix, the generated code looks like so:
The added
Typesuffix ensures the names do not conflict, solving any build issues that would otherwise arise.@coderabbitai summary