Skip to content

fix(typescript): apply exactOptionalPropertyTypes to inline request types#16972

Open
dg314 wants to merge 1 commit into
fern-api:mainfrom
dg314:ts-sdk/request-wrapper-exact-optional
Open

fix(typescript): apply exactOptionalPropertyTypes to inline request types#16972
dg314 wants to merge 1 commit into
fern-api:mainfrom
dg314:ts-sdk/request-wrapper-exact-optional

Conversation

@dg314

@dg314 dg314 commented Jul 8, 2026

Copy link
Copy Markdown

What

With noSerdeLayer: true, optional properties on inline request types (client/requests/*) are now generated as propName?: Type | undefined, matching what the object type generator already does for type declarations.

Why

The exactOptionalPropertyTypes support (added in 3.47.0) only reached the object type generator. Request wrappers still emitted plain propName?: Type, so under exactOptionalPropertyTypes: true a caller couldn't pass a value that explicitly sets an optional field to undefined (e.g. spreading an object where optionals are typed T | undefined) — even though the referenced schema type accepted it. This makes the two consistent.

Change

One emission point in GeneratedRequestWrapperImpl.writeToFile: when a property is optional and the serde layer is off, widen its type node to T | undefined (same !includeSerdeLayer gate the object generator uses).

Made with Cursor


Open in Devin Review

…ypes

Optional request-wrapper properties now emit `T | undefined` when
`noSerdeLayer: true`, mirroring the object type generator.
@dg314 dg314 requested a review from Swimburger as a code owner July 8, 2026 20:04

@nitpickybot nitpickybot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review Summary

The PR widens optional inline request properties to T | undefined when the serde layer is off, mirroring the object type generator. The change is small and consistent with the stated intent. Minor concern about referencing the includeSerdeLayer field.

  • 🔵 2 suggestion(s)

// When the serde layer is disabled, widen optional properties to `T | undefined`
// (mirroring the object type generator) so they satisfy exactOptionalPropertyTypes.
const typeNode =
property.isOptional && !this.includeSerdeLayer

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion

Verify this.includeSerdeLayer is the correct field. The description says the object generator gates on !includeSerdeLayer, but double-check the exact property name/accessor used elsewhere in this class (e.g. this.includeSerdeLayer vs a context-derived flag) to keep behavior identical to the object type generator.

object type generator. Previously only type declarations got the `| undefined`, so request
wrappers were unassignable from values that explicitly set optional fields to `undefined`.
type: fix
createdAt: "2026-07-08"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion

createdAt: "2026-07-08" — that's a year in the future. Presumably meant 2025.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 No seed test output updates included for the generator behavior change

The PR changes generated output for SDKs using noSerdeLayer: true with optional request properties, but doesn't include any seed test fixture updates. If there are existing seed fixtures that exercise noSerdeLayer: true with optional request wrapper properties, their outputs would now differ. This may be intentional (seed updates done separately) or may indicate the change isn't exercised by existing fixtures.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +133 to +138
const typeNode =
property.isOptional && !this.includeSerdeLayer
? ts.factory.createUnionTypeNode([
property.type,
ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword)
])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Optional file upload properties get a redundant duplicate | undefined in generated type output

The optional-property widening (GeneratedRequestWrapperImpl.ts:133-138) wraps property.type with | undefined, but for file upload properties getFileParameterType (GeneratedRequestWrapperImpl.ts:930-931) already appends | undefined when the file is optional, so the generated type becomes e.g. Uploadable | undefined | undefined.

Impact: Users with noSerdeLayer: true and optional file uploads see redundant | undefined | undefined in generated request interfaces.

Mechanism: getFileParameterType already adds undefined for optional files

In getFileParameterType at generators/typescript/sdk/request-wrapper-generator/src/GeneratedRequestWrapperImpl.ts:930-931:

if (property.isOptional) {
    types.push(ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword));
}

Then in getRequestProperties at line 313-314, the file property is pushed with:

type: this.getFileParameterType(fileProperty, context),
isOptional: fileProperty.isOptional,

So property.type already contains | undefined and property.isOptional is true. The new code at lines 133-138 then wraps this again with | undefined.

The object type generator (generators/typescript/model/type-generator/src/object/GeneratedObjectTypeImpl.ts:71-91) prevents this by calling stripTopLevelUndefined before adding | undefined. The request wrapper should do the same, or should skip the widening for file properties whose type already includes undefined.

Prompt for agents
The new code at lines 133-138 in writeToFile wraps property.type with | undefined when property.isOptional && !this.includeSerdeLayer. However, for file upload properties, getFileParameterType (line 909-935) already adds | undefined to the type when the file is optional. This produces redundant Uploadable | undefined | undefined in generated output.

To fix this, either:
1. Strip top-level undefined from property.type before wrapping (like GeneratedObjectTypeImpl does with its stripTopLevelUndefined helper), OR
2. Don't add | undefined in getFileParameterType when property.isOptional is true (and rely on the writeToFile widening instead), OR
3. Add a flag to the Property interface indicating whether the type already includes undefined, and skip widening in that case.

Option 1 is the most consistent with the existing object type generator pattern. You'd need to add a stripTopLevelUndefined helper (or import/share the one from GeneratedObjectTypeImpl) and call it on property.type before creating the union with undefined.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant