feat: Replace socket.io-client #99

gismya · 2023-04-19T02:17:51Z

I have added automatic tests where applicable
The PR title is suitable as a release note
The PR contains a description of what has been changed
The description contains manual test instructions

Changes

Removes the old socket.io client and writes a new implementation using the same communication protocol. Re-implements the parts that we were using, and scraps the rest. None of the removed parts were documented, but it was parts that might have been found by people implementing the API, so it will be released as a part of the V2 release with other breaking changes.

Resolves #90

Test

Make sure the code looks good and the test cases seem to cover everything we need. Functionality can be tested in the application by yarn linking.

package.json

source/simple_socketio.ts

jimmycallin · 2023-04-19T08:31:35Z

source/simple_socketio.ts

+      args: [eventData],
+    };
+    const dataString = eventData ? `:::${JSON.stringify(payload)}` : "";
+    this.ws?.send(`${PACKET_TYPES.event}${dataString}`);


it feels a bit wrong to do nothing if the ws isn't initialized. what did the previous client do?

Don't think it was handled, the event_hub code does unsent event queueing. But I can investigate

I've added a simple queue

source/simple_socketio.ts

gismya · 2023-05-12T17:18:49Z

I think I've fixed the reconnect behavior now, but didn't have time to properly test it. I'll also have to look at the code again later to see if it needs to be cleaned up.

ffMathy · 2023-05-13T04:37:25Z

That's awesome stuff! I'll throw an additional review at it ASAP.

ffMathy · 2023-05-15T11:45:55Z

LGTM! 😍

emm4back · 2023-05-15T11:58:12Z

test/event_hub.test.js

@@ -0,0 +1,32 @@
+import { EventHub } from "../source/event_hub";


Missing Copyright text

ffMathy

Only one minor comment.

ffMathy · 2023-05-16T12:44:57Z

source/event_hub.ts

@@ -247,7 +237,7 @@ export class EventHub {
   *
   * If timeout is non-zero, the promise will be rejected if the event is not
   * sent before the timeout is reached. Should be specified as seconds and
-   * will default to 10.
+   * will default to 30.


Isn't this a breaking change?

It was simply my noticing that the comment was incorrect.

Ah okay. All good then!

jimmycallin · 2023-05-17T09:41:58Z

package.json

+    "vite": "^4.2.2",
+    "vite-plugin-dts": "^2.3.0",
+    "vitest": "^0.30.1",
+    "ws": "^8.13.0"


please remove unrelated dependency upgrades and make them in a separate PR

ffMathy · 2023-05-22T08:49:21Z

Loving the progress so far! We're cheering for you in my team. Keep up the good work! ❤️

ffMathy · 2023-05-23T10:29:30Z

Yaaaay!

This reverts commit ceb68a7.

* Revert "fix(test): Fix Math.random flaky test (#108)" This reverts commit 0a140a7. * Revert "feat: Replace socket.io-client (#99)" This reverts commit ceb68a7.

gismya and others added 23 commits April 18, 2023 14:42

Initial test

5b879be

Comments

24828db

Fetch error handling

67f6ee3

Separate handle methods

e83367d

Comments and code clarity

f82e685

Fetch timeout

ee02b81

Add error event handler

ca3335c

Change error handler to use handleClose

19e57b7

Added error packet type handler

176a79b

Setup connect event

3144260

accidentally removed the initialization

cd01dbb

Few tests, some working some not

698e989

rename

0d34757

rename

296e94b

Optional chaining

91c24d0

Add https version of needed endpoint

d20f3b3

Export packet_types for use in tests

c91be00

Round of test writing

e6468ac

DRYer code

d6b1db0

Improve comments

476cc63

Further test improvements

c834a91

A bunch more tests

b685485

Remove non working attempt to setup a mock websocket server

5c739b5

gismya requested a review from a team as a code owner April 19, 2023 02:17

gismya marked this pull request as draft April 19, 2023 02:18

jimmycallin reviewed Apr 19, 2023

View reviewed changes

gismya added 4 commits April 19, 2023 11:17

Include external isomorphic-ws

8da3169

Move ws to peer dependencies

46a30bc

Add tsdoc comments

0f4e38d

DRYer msw server

9111ff6

gismya marked this pull request as ready for review May 13, 2023 23:29

emm4back reviewed May 15, 2023

View reviewed changes

test/event_hub.test.js Outdated

@@ -0,0 +1,32 @@

import { EventHub } from "../source/event_hub";

Copy link

emm4back May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Copyright text

gismya changed the base branch from main to release/v2 May 15, 2023 12:14

gismya linked an issue May 15, 2023 that may be closed by this pull request

What is preventing Event Hub from working in Node? #90

Closed

ffMathy approved these changes May 16, 2023

View reviewed changes

jimmycallin approved these changes May 19, 2023

View reviewed changes

gismya changed the base branch from release/v2 to main May 19, 2023 12:19

gismya force-pushed the rewrite-socketio-client branch from 68c2591 to c4f2e24 Compare May 19, 2023 12:23

jimmycallin changed the title ~~feat!: Replace socket.io-client~~ feat: Replace socket.io-client May 19, 2023

renovate bot and others added 8 commits May 22, 2023 09:56

chore(deps): update dependency prettier to v2.8.8 (#102)

96ae6bf

chore(deps): update dependency vite-plugin-dts to v1.7.3 (#103)

453ba50

Fixed the reconnect test

f733d34

Add a tsdoc

230b9fc

cleanup

7c9fff7

Improve backwards compatibility

3bf55f4

Updated tests

3f4f3e8

Change open to a getter

a7834e5

gismya force-pushed the rewrite-socketio-client branch from c4f2e24 to a7834e5 Compare May 22, 2023 07:58

gismya added 2 commits May 23, 2023 12:01

Merge remote-tracking branch 'origin/main' into rewrite-socketio-client

3491353

Upgrade

f9a0bd8

gismya merged commit ceb68a7 into main May 23, 2023

gismya deleted the rewrite-socketio-client branch May 23, 2023 10:08

lucaas added a commit that referenced this pull request May 26, 2023

Revert "feat: Replace socket.io-client (#99)"

20c0607

This reverts commit ceb68a7.

lucaas mentioned this pull request May 26, 2023

fix: Revert "Replace socket.io-client" #111

Merged

4 tasks

lucaas added a commit that referenced this pull request May 26, 2023

fix: Revert "Replace socket.io-client" (#111)

ca25de7

* Revert "fix(test): Fix Math.random flaky test (#108)" This reverts commit 0a140a7. * Revert "feat: Replace socket.io-client (#99)" This reverts commit ceb68a7.

		@@ -0,0 +1,32 @@
		import { EventHub } from "../source/event_hub";

feat: Replace socket.io-client #99

feat: Replace socket.io-client #99

Uh oh!

Conversation

gismya commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gismya commented May 12, 2023

Uh oh!

ffMathy commented May 13, 2023

Uh oh!

ffMathy commented May 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffMathy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffMathy commented May 22, 2023

Uh oh!

ffMathy commented May 23, 2023

Uh oh!

Uh oh!

gismya commented Apr 19, 2023 •

edited

Loading