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

Add negotiate #710

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add negotiate #710

wants to merge 2 commits into from

Conversation

zackliu
Copy link

@zackliu zackliu commented Sep 19, 2023

Note: the engine.io.js file is the generated output of make engine.io.js, and should not be manually modified.

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

The engine.io connection directly connects to the hostname, path.

New behaviour

If negotiatePath is set, engine.io will make a fetch to the <hostname>/<negotiatePath> to get a JSON encoded response first.

{
  "url", "http://hostname/path",
  "token": "<token>"
}

if succeeded, the engine.io will parse the url got in negotiation and replace it with the previous hostname, path, port and protocol in opts.
As the opts has changed, everything is unchanged then (e.g when constructing a transport, it still read from opts).

Another change is adding a token in opts. If token is set, it will be used in Authorization for long polling, and access_token query for websocket. This token can be set in opts directly but it can also be override in negotiation.

Other information (e.g. related issues)

This is the implementation of issue socketio/socket.io#4825

@zackliu zackliu marked this pull request as ready for review September 21, 2023 06:52
@zackliu
Copy link
Author

zackliu commented Sep 21, 2023

@darrachequesne We can discuss whether the design is acceptable, and I will write some more test.

@zackliu
Copy link
Author

zackliu commented Oct 12, 2023

@darrachequesne Do you have any comments on the design?

@darrachequesne
Copy link
Member

Hi @zackliu,

Thanks for your work on this.

However, I still don't think the use case is common enough for this to be included here in the package.

@zackliu
Copy link
Author

zackliu commented Oct 23, 2023

@darrachequesne I think there're two parts in it.

  1. Added a token in opts, which can be used in Authorization in long polling and access_token query in websocket
  2. Add a negotiate opts and replace the correlating fields in opts.

I admit we can use a separate library for the second purpose but what about the first one? Do you think it's acceptable to have such a property as it's hard to achieve outside this package?

@darrachequesne
Copy link
Member

For 1, the transportOptions option should be sufficient I think:

import { io } from "socket.io-client";

const socket = io({
  transportOptions: {
    polling: {
      extraHeaders: {
        Authorization: myToken
      }
    },
    websocket: {
      query: {
        access_token: myToken
      }
    }
  }
});

Reference: https://socket.io/docs/v4/client-options/#transportoptions

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.

None yet

2 participants