Repository navigation
fix(typescript): match SDK serde default, quote non-identifier keys and add file placeholder in dynamic snippets - #18323
willkendall01 wants to merge 2 commits into
Conversation
… non-identifier keys, add file placeholder Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
There was a problem hiding this comment.
AI Review Summary
The change flips the dynamic-snippet noSerdeLayer default to true, quotes non-identifier property keys, and emits an fs.createReadStream placeholder for missing file values. The default flip is behaviorally correct per the SDK generator, but note the snapshot now shows maybe_file (an optional file property) receiving a placeholder, which looks like an unintended widening beyond required files; worth confirming. Otherwise the regex/JSON.stringify quoting and the Blob unification look sound.
- 🟡 1 warning(s)
- 🔵 1 suggestion(s)
Review Scope
- Mode: Full pull request
- Range:
Full pull request through fbb0144d13a039ada15dbd175cdd7ac77b8a8542 - Current range coverage: Complete
- Cumulative pull request coverage: Complete
This review is complete for the current scope. Addressing all critical and warning findings above satisfies the review; suggestions are optional. Request another review only if subsequent changes introduce new behavior or materially expand the pull request's scope.
| const fileValue = this.context.getSingleFileValue({ property, record }); | ||
| if (fileValue == null) { | ||
| return ts.TypeLiteral.nop(); | ||
| return this.context.getFilePlaceholder(); |
There was a problem hiding this comment.
🟡 warning
This branch fires for any file property with no example value, including optional ones. The snapshot confirms it: maybe_file: fs.createReadStream("/path/to/your/file") now shows up in the file-upload snippet even though the example omitted it. The PR description says the goal was to stop dropping required file fields; for optional ones the previous nop() (omit the key) was the right output. Consider gating on property.isOptional/required-ness and keeping the placeholder only for required properties.
There was a problem hiding this comment.
This is deliberate. The dynamic IR's FileUploadRequestBodyProperty.file is only a NameAndWireValue, with no optional/required flag, so the snippet generator can't tell maybe_file from file. A placeholder on an optional field still compiles and is a valid call. Leaving out a required one doesn't compile. The Python and Go dynamic snippets do the same thing (they emit a placeholder for any file field without an example value). If optionality is added to the IR later, this can be limited to required fields.
| import { DynamicTypeLiteralMapper } from "./DynamicTypeLiteralMapper.js"; | ||
| import { FilePropertyMapper } from "./FilePropertyMapper.js"; | ||
|
|
||
| const VALID_IDENTIFIER_REGEX = /^[A-Za-z_$][A-Za-z0-9_$]*$/; |
There was a problem hiding this comment.
🔵 suggestion
VALID_IDENTIFIER_REGEX accepts JS reserved words (class, default, function), which are legal as unquoted object-literal keys, so output stays valid — just flagging that the name is slightly aspirational. Renaming to OBJECT_KEY_IDENTIFIER_REGEX would make the intent (valid property key, not valid binding identifier) clearer.
There was a problem hiding this comment.
Agreed, reserved words are valid unquoted keys, so the output is correct. I'm keeping the name to avoid churn. The comment at the call site already says it's about property keys.
There was a problem hiding this comment.
👀 1 finding needs your review
Devin fixed 1 of 2 findings on fbb0144. Click a finding below to jump to its comment.
For your review (1)
Fixed by Devin (1)
- Wire-name overrides break default snippets
View all findings in Devin Review
…serde layer in dynamic snippets Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| const fileValue = this.context.getSingleFileValue({ property, record }); | ||
| if (fileValue == null) { | ||
| return ts.TypeLiteral.nop(); | ||
| return this.context.getFilePlaceholder(); |
There was a problem hiding this comment.
🔴 Absent optional files become failed uploads
When an optional file is absent, getSingleFileProperty inserts a stream for /path/to/your/file. The SDK uploads that nonexistent path instead of omitting the file, so the snippet fails when run.
Learn more
File-upload properties can be optional in the generated SDK; its request interface marks maybe_file optional and its upload method skips absent values. getSingleFileValue returns undefined for both omitted properties and invalid values. The unconditional fallback therefore inserts a stream even when the example intentionally omits an optional file. A missing placeholder path later fails during upload.
Example: An example supplies file: "Hello" but omits optional maybe_file. The snippet emits maybe_file: fs.createReadStream("/path/to/your/file") instead of omitting maybe_file.
Recommended fix: Preserve omission for optional files while inserting a placeholder only for required files with no example. Carry file optionality into the dynamic IR if it is not currently available, and avoid replacing invalid supplied values with a placeholder.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Same limitation as the earlier thread. The dynamic IR's file properties don't say whether they're optional, so this can't be limited to required files without an IR change, and that would be a separate CLI/IR PR. A placeholder on an optional file still compiles and is meant to be replaced, while leaving out a required one doesn't compile. Python and Go do the same. Leaving the decision on scope to a maintainer.
The other finding from this review (name overrides without a serde layer) is fixed in 95c153c: query params, headers, file fields and object properties now use the wire value, as the SDK does.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Description
Linear ticket: N/A
TypeScript dynamic snippets didn't compile against SDKs generated from the same config:
resolveNoSerdeLayertreated an omittednoSerdeLayerasfalse. The SDK generator treats it astrue, so snippets used camelCase request properties (perPage) while the SDK types use wire names (per_page).formatOriginalPropertyNameonly quoted names containing-, so keys likeorg.iso.18013.5.1.mDLorx5t#S256came out unquoted, which is a syntax error.TypeLiteral.nop()), so the call was missing a required property.Changes Made
resolveNoSerdeLayernow defaults totrue, the same as the SDK generator (serdeLayerstill takes precedence). This changes dynamic snippet output only, not generated SDK code./^[A-Za-z_$][A-Za-z0-9_$]*$/is now quoted withJSON.stringify.fs.createReadStream("/path/to/your/file").new Blob([...]). ThenoSerdeLayerbranch emitted a bare string, whichcore.file.Uploadabledoesn't accept. With the new default, that branch would have run for most configs.generators/typescript/sdk/changes/unreleased/dynamic-snippets-sdk-defaults-and-files.ymlTesting
@fern-api/typescript-dynamic-snippetssnapshots: wire property names, file placeholder)noSerdeLayer-omitted, dotted-key and required-file checks fail on the published package and pass on this build.Link to Devin session: https://app.devin.ai/sessions/e71a3ab9f283405cb547310ce6dac721
Open in Devin Desktop: https://app.devin.ai/desktop/session/e71a3ab9f283405cb547310ce6dac721?variant=devin
Requested by: @willkendall01