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

feat(socketioxide/v4): implement SocketIO V4 (closes #51) #52

Merged
merged 29 commits into from
Oct 17, 2023

Conversation

sleeyax
Copy link
Contributor

@sleeyax sleeyax commented Jul 3, 2023

Protocol implementation steps:

  • Add implicit connection to the default namespace
  • rename CONNECT_ERROR to ERROR Not necessary because the value hasn't changed, only the name.
  • The CONNECT packet shouldn't contain a payload
  • The payload CONNECT_ERROR should be a plain string instead of an object. I might skip implementing this, I think a v4 client currently would receive a JSON string as the error, which isn't a big deal.
  • Add E2E tests (WIP: https://github.com/sleeyax/socket.io-protocol/tree/v4-testsuite)

@sleeyax sleeyax force-pushed the ft-socketio-v4-support branch from ec0ee89 to 5cd8ea5 Compare July 3, 2023 14:47
@sleeyax sleeyax force-pushed the ft-socketio-v4-support branch from 5cd8ea5 to abf75d0 Compare July 3, 2023 18:29
In the v4 protocol, CONNECT packets don't have nor support payloads ('data' field).
@sleeyax
Copy link
Contributor Author

sleeyax commented Jul 3, 2023

I'm having a hard time figuring out an annoying issue with tests. Sometimes the tests fail and sometimes they don't 🤔

It's likely related to this part of the implementation:

Add implicit connection to the default namespace

I'm not sure how tests should behave when the client is already connected to the default namespace when the connection is made. Maybe I'm encountering a race condition? Or perhaps my server implementation is just wrong. Either way, I would appreciate it if someone with some time to spare could have a look and help me out, I need to take a break from this :)

Example to reproduce

Server:

cargo run --bin socketioxide-e2e --all-features

Client:

npm test -- -f "should allow connection to a custom namespace"

    // ...
    it("should allow connection to a custom namespace", async () => {
      for (let i = 0; i < 3; i++) {
        console.log(i); // works at i = 0, fails at i = 1

        const socket = new WebSocket(
          `${WS_URL}/socket.io/?EIO=3&transport=websocket`
        );
  
        await waitFor(socket, "message"); // Engine.IO handshake

        socket.send("40/custom");
  
        const { data } = await waitFor(socket, "message");
  
        expect(data).to.startsWith("40/custom");
      }
    });

@Totodore Totodore changed the title Feature: implement SocketIO V4 (closes #51) feat(socketioxide/v4): implement SocketIO V4 (closes #51) Jul 29, 2023
@sleeyax
Copy link
Contributor Author

sleeyax commented Jul 30, 2023

Hey all, just wanted to mention that I'm currently busy with other projects. I'm not sure when I'll be able to get back to this PR. If anyone wants to take over, definitely don't wait for me. Cheers!

@Totodore
Copy link
Owner

Totodore commented Oct 14, 2023

I'm having a hard time figuring out an annoying issue with tests. Sometimes the tests fail and sometimes they don't 🤔

It's likely related to this part of the implementation:

Add implicit connection to the default namespace

I'm not sure how tests should behave when the client is already connected to the default namespace when the connection is made. Maybe I'm encountering a race condition? Or perhaps my server implementation is just wrong. Either way, I would appreciate it if someone with some time to spare could have a look and help me out, I need to take a break from this :)

Example to reproduce

Server:

cargo run --bin socketioxide-e2e --all-features

Client:

npm test -- -f "should allow connection to a custom namespace"

    // ...
    it("should allow connection to a custom namespace", async () => {
      for (let i = 0; i < 3; i++) {
        console.log(i); // works at i = 0, fails at i = 1

        const socket = new WebSocket(
          `${WS_URL}/socket.io/?EIO=3&transport=websocket`
        );
  
        await waitFor(socket, "message"); // Engine.IO handshake

        socket.send("40/custom");
  
        const { data } = await waitFor(socket, "message");
  
        expect(data).to.startsWith("40/custom");
      }
    });

A bit late but it corresponds to this issue : socketio/socket.io-protocol#32

@Totodore Totodore marked this pull request as ready for review October 15, 2023 13:32
@tausifcreates
Copy link
Contributor

Is it necessary to support a legacy protocol?

@Totodore
Copy link
Owner

Is it necessary to support a legacy protocol?

@sleeyax made 90% of the work, it is not a big deal to merge this PR

@sleeyax
Copy link
Contributor Author

sleeyax commented Oct 16, 2023

Is it necessary to support a legacy protocol?

I personally needed this to support a legacy socketio client that pretty much can't upgrade to the latest protocol version any time soon. I'm sure others will have similar use cases in the future for socketioxide and given you can leverage Rust feature flags I don't see a reason not to support older protocols. And finally, like totodore said most of the work for V4 is done already anyways :)

@tausifcreates
Copy link
Contributor

Is it necessary to support a legacy protocol?

I personally needed this to support a legacy socketio client that pretty much can't upgrade

Makes perfect sense, thanks for explaining @sleeyax

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@Totodore Totodore merged commit b72489f into Totodore:main Oct 17, 2023
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.

3 participants