-
-
Notifications
You must be signed in to change notification settings - Fork 100
Run hooks with websockets and rewrite ws headers #292
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
Conversation
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few tweaks/questions
Co-authored-by: Frazer Smith <[email protected]>
@Fdawgs ptal |
Signed-off-by: Matteo Collina <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the docs front. Don't know enough about websockets to comment on functionality, but will leave that to the others to review!
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
Note that this also fixes a regression for the getUpstream() support for Websocket that was changed in fastify/fastify-reply-from#293 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor things - logic
Do you agree to release it as major?
if (request.headers.cookie) { | ||
return { ...headers, cookie: request.headers.cookie } | ||
} | ||
return { ...headers } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { ...headers } | |
return headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deliberate to create a new object.
Co-authored-by: Manuel Spigolon <[email protected]>
Co-authored-by: Manuel Spigolon <[email protected]>
Yes, this should be a major. |
Co-authored-by: Manuel Spigolon <[email protected]>
Checklist
npm run test
andnpm run benchmark
and the Code of conduct