Skip to content

Conversation

@tjb9dc
Copy link
Collaborator

@tjb9dc tjb9dc commented Dec 5, 2025

Description

Refs: Follow-up to #10966

Updates WebSocket ConnectArgs interface to conditionally include undefined in the union type for allow-multiple: true query parameters based on whether the parameter is optional:

  • Non-optional, allow-multiple: foo: T | T[]
  • Optional, allow-multiple: foo?: T | T[] | undefined

Link to Devin run: https://app.devin.ai/sessions/3df4d8e6eeef4b0bb25291da9228d746
Requested by: [email protected] (@tjb9dc)

Changes Made

  • Updated GeneratedDefaultWebsocketImplementation.ts to conditionally add undefined to the union type for allow-multiple query parameters only when context.type.isOptional(queryParameter.valueType) is true
  • Added version 3.35.10 changelog entry

Updates Since Last Revision

Based on reviewer feedback, changed from always adding undefined to only adding it when the query parameter's value type is optional.

Testing

  • Lint checks pass (pnpm run check)

Human Review Checklist

  • Verify the conditional behavior (only adding undefined when optional) is the intended behavior
  • Note: The changelog entry in versions.yml says T | T[] | undefined unconditionally - may want to update it to reflect the conditional behavior
  • Confirm this differs from GeneratedRequestWrapperImpl.ts (lines 222-227) which uses T | T[] without explicit undefined - is this intentional divergence?

@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Comment on lines 179 to 180
ts.factory.createArrayTypeNode(type.typeNodeWithoutUndefined),
ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

um I guess this works? Should we instead be keying off of whether this allows undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point—updated to only include undefined in the union when the query parameter's value type is optional per context.type.isOptional(...). This gives us:

  • Non-optional, allow-multiple: foo: T | T[]
  • Optional, allow-multiple: foo?: T | T[] | undefined

@tjb9dc tjb9dc enabled auto-merge (squash) December 10, 2025 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants