Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(websockets): improve type safety for WebSocketGateway options #14305

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sapenlei
Copy link
Contributor

@sapenlei sapenlei commented Dec 10, 2024

Closes #14303

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #14303

What is the new behavior?

improve type safety for WebSocketGateway options

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

Pull Request Test Coverage Report for Build c9edbf1d-dcc3-4eba-8f47-e0946aebec59

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.912%

Totals Coverage Status
Change from base Build bcb74b8d-8301-431a-a72f-c672ad1a017b: 0.0%
Covered Lines: 6818
Relevant Lines: 7418

💛 - Coveralls

@sapenlei
Copy link
Contributor Author

@kamilmysliwiec

@micalevisk
Copy link
Member

micalevisk commented Dec 11, 2024

FYI this does introduces a breaking change as we'll narrow the type in a public API

@sapenlei
Copy link
Contributor Author

FYI this does introduces a breaking change as we'll narrow the type in a public API

I don't think there's a breaking change. Here's where options are really used.
CleanShot 2024-12-12 at 11 50 12@2x

@sapenlei
Copy link
Contributor Author

The type of WebSocketGateway options is T extends Record<string, any> = GatewayMetadata , There's no difference between this and T extends Record<string, any>

@kamilmysliwiec
Copy link
Member

@sapenlei this introduces a breaking change as you added T extends GatewayMetadata which is a socket.io options object, not Record<string, any>

export function WebSocketGateway<
T extends Record<string, any> = GatewayMetadata,
>(portOrOptions?: number | T, options?: T): ClassDecorator {
export function WebSocketGateway<T extends GatewayMetadata>(
Copy link
Member

Choose a reason for hiding this comment

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

we can't do extends GatewayMetadata here as GatewayMetadata interface is specific to socket.io. What if someone uses ws instead? (a different adapter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Even for ws, it still calls this method

Copy link
Member

Choose a reason for hiding this comment

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

We should update the generic argument of this method then (as T is not always a subset of GatewayMetadata)

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.

The type of WebSocketGateway's options does not work
4 participants