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

Fix query when multiplexing (#3495) #3496

Closed
wants to merge 2 commits into from
Closed

Fix query when multiplexing (#3495) #3496

wants to merge 2 commits into from

Conversation

sebamarynissen
Copy link
Contributor

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

When multiplexing is turned on - which is the default - the query sent to the server when connecting to another namespace is incorrectly processed (as described in #3495).

New behaviour

The query params are correctly overriden on the server.

Other information (e.g. related issues)

See #3495.

@sebamarynissen
Copy link
Contributor Author

I wanted to add a test for this specific case, but I couldn't really figure out how to to do it since the tests don't include the client library.

In any case, all existing tests still pass.

darrachequesne pushed a commit that referenced this pull request Jan 4, 2021
The `query` option of the Manager had the priority over the one of the
Socket instance, which meant updating the Socket#query object on the
client-side was not reflected in the Socket#handshake object on the
server-side.

Please note that the behavior of the `query` option is still a bit
weird in Socket.IO v2, as it only applies to non-default namespace.
This is fixed in v3:

- https://socket.io/docs/v3/migrating-from-2-x-to-3-0/#Add-a-clear-distinction-between-the-Manager-query-option-and-the-Socket-query-option
- https://socket.io/docs/v3/middlewares/#Sending-credentials

Fixes #3496
@darrachequesne
Copy link
Member

Merged as d33a619 (and included in [email protected]).

Thanks!

@darrachequesne darrachequesne added this to the 2.4.0 milestone Jan 4, 2021
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.

2 participants