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

Adding an option to avoid the added trailing slash to the path #694

Closed
wants to merge 1 commit into from

Conversation

iifawzi
Copy link
Contributor

@iifawzi iifawzi commented Oct 15, 2022

Signed-off-by: iifawzi iifawzie@gmail.com

Hi, this PR is adding an option to avoid the added trailing slash to the path in the connection
Resolving socketio/socket.io-client#1550.

An example of a use case for this is the Microsoft bot framework stream URL:
Reconnect to a conversation in Direct Line API 3.0

io(`wss://directline.botframework.com/v3/directline/conversations/convId/stream?t=tokenId`)

I needed to use the native WebSocket since the engine.io-client is always adding a trailing slash after stream which broke the URI and hence the connection.

Having this as an optional option would solve such use cases.

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

Current behaviour

a trailing slash is always added to the path.

New behaviour

a trailing slash is still added to the path, but can be avoided by setting the trailingSlash option to false.

Signed-off-by: iifawzi <iifawzie@gmail.com>
@darrachequesne
Copy link
Member

darrachequesne commented Oct 17, 2022

Hi! Thanks for the pull request.

I guess we would need the same option on the server side, right? See here: https://github.com/socketio/engine.io/blob/917d1d29e13f2e8f523c3738f6413f67b587aebe/lib/server.ts#L642-L647

Regarding your use case, I think the Microsoft bot framework exposes a classic WebSocket server, and not a Socket.IO one, so you won't be able to connect anyway. Or am I missing something?

@iifawzi
Copy link
Contributor Author

iifawzi commented Oct 17, 2022

Hi @darrachequesne, regarding the server side, I think yes, I didn't notice that, we need to have that check there too. I will double confirm that, and work on creating a PR there as well, later today

Regarding the framework, you're right. But don't you feel that having trailingSlash as an option would be a great addition? maybe some people are following the same URI pattern, wdyt?

@darrachequesne
Copy link
Member

Hmm, I'm not really sure about the usefulness of this new option. If others could chime in...

@iifawzi
Copy link
Contributor Author

iifawzi commented Oct 18, 2022

IMHO, the URI pattern that Microsoft uses is legitimate, and might be used by others as mentioned in socketio/socket.io-client#1550 and socketio/socket.io-client-swift#297.

Why the backslash is added from the first place? Is there a specific need for it?

@binishp
Copy link

binishp commented Oct 21, 2022

I have the exact issue with the library. The network firewalls are stripping of trailing slashes resulting in socket connectivity problem. I would definately love this option.

@DylanCulfogienis
Copy link

We're currently having issues with this in our application. Lots of proxies and frontend libraries (Next.js, http-proxy) strip the trailing slash for all URLs that get rewritten, which results in Socket.io specifically not working.

Having an option to disable the trailing slash, or better yet just making it so setting path will not automatically append a trailing slash, would be great for compatibility with these technologies.

@iifawzi
Copy link
Contributor Author

iifawzi commented Dec 7, 2022

Hi @darrachequesne, would really appreciate it, if you can take another look here.

darrachequesne pushed a commit that referenced this pull request Dec 9, 2022
The "addTrailingSlash" option allows to control whether a trailing
slash is added to the path of the HTTP requests created by the library:

- true (default): "/engine.io/"
- false: "/engine.io"

Related: socketio/socket.io-client#1550

Signed-off-by: iifawzi <iifawzie@gmail.com>
@darrachequesne
Copy link
Member

Merged as 21a6e12. Thanks 👍

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.

4 participants