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(@nestjs/websocket) ws support for connection req #821

Closed
wants to merge 5 commits into from
Closed

refactor(@nestjs/websocket) ws support for connection req #821

wants to merge 5 commits into from

Conversation

tannineo
Copy link

@tannineo tannineo commented Jun 28, 2018

add a second arg of handleConnection to support (client, req) => {} callback of the connection event

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)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Current handleConnection of the websocket gateway just apply 1 arg client, which depends on different websocket implements. For socket.io there is only client, but for ws, there're 2 args of the connection event: client, req.

Issue Number: 813

What is the new behavior?

handleConnection now accept 2 args.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@tannineo
Copy link
Author

I wonder why npm ci takes so long while npm i on my machine completes successfully...
Need help~

return (client, req) => {
// here different websocket implements may have different args
// `ws` will have two args applied to this callback
connection.next({ client, req });
Copy link
Member

Choose a reason for hiding this comment

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

since we don't know the specific list of arguments, maybe we should pass all of them (using rest operator) instead of 2 selected ones

Copy link
Author

Choose a reason for hiding this comment

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

you are right, I will try to fix.

add a second arg of `handleConnection` to support  (client, req) => {} callback of the `connection` event
@coveralls
Copy link

coveralls commented Jul 6, 2018

Coverage Status

Coverage decreased (-0.03%) to 94.623% when pulling e3b9122 on tannineo:813-ws-req-on-connection-fix into 8d62307 on nestjs:master.

@tannineo
Copy link
Author

tannineo commented Jul 6, 2018

I try to change the code using ... to pass the rest of args and modify a test case.
The coverage falls... I'll try to cover those later...

tannineo added 3 commits July 9, 2018 13:52
update to nestjs/nest/master
add a second arg of `handleConnection` to support  (client, req) => {} callback of the `connection` event
@kamilmysliwiec
Copy link
Member

Any updates here? cc @tannineo

@kamilmysliwiec
Copy link
Member

See #813

@lock
Copy link

lock bot commented Sep 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants