[WALL-25371] Remove RxJS from WalletLinkConnection #1018

bangtoven · 2023-09-29T08:36:17Z

Summary

removed RxJS and transitioned to TS promises while preserving the original data flow as closely as possible.
please note that we prioritized maintaining consistency with the previous rxjs-based implementation.
we will revisit for a cleaner implementation with simpler logic.

Previous RxJS removal changes
#779
#969
#988
#992

How did you test your changes?

manual + unit tests

test.mov

This reverts commit c3ae669.

cb-jake · 2023-09-29T20:09:42Z

packages/wallet-sdk/src/connection/RxWebSocket.test.ts


-import { ConnectionState, RxWebSocket } from './RxWebSocket';
+import { ConnectionState, WalletLinkWebSocket } from './RxWebSocket';


Should we rename this file name to WalletLinkWebSocket?

yes indeed.
didn't make the change yet due to github diff issues. will address in a later pr.

packages/wallet-sdk/src/connection/RxWebSocket.ts

packages/wallet-sdk/src/connection/WalletLinkConnection.ts

cb-jake · 2023-09-29T20:54:29Z

🙌 Looks great! Will need to pull it down and test. I'll check it out on Monday!

bangtoven · 2023-10-06T23:30:05Z

packages/wallet-sdk/package.json

@@ -43,7 +43,6 @@
    "eventemitter3": "^5.0.1",


dear reviewers,
recommend to turn on Hide whitespace
https://github.com/coinbase/coinbase-wallet-sdk/pull/1018/files?diff=split&w=1
ty!

cb-jake · 2023-10-10T17:12:40Z

packages/wallet-sdk/src/connection/RxWebSocket.ts

@@ -1,27 +1,29 @@
 // Copyright (c) 2018-2023 Coinbase, Inc. <https://www.coinbase.com/>
 // Licensed under the Apache License, version 2.0

-import { BehaviorSubject, empty, Observable, of, Subject, throwError } from 'rxjs';
-import { flatMap, take } from 'rxjs/operators';
+import { ServerMessage } from './ServerMessage';


nit: should we rename this file to be WebSocket since we're no longer using RX?

#1018 (comment)
@cb-jake yup. but then github considered it as a new file and made it hard to compare from the previous implementation. will rename the file as followup.

that makes sense

cb-jake

This looks good and testing seems to work. I think we should have someone with more rxjs experience should look at the PR before merging

hieronymus777 · 2023-10-27T16:28:28Z

packages/wallet-sdk/src/connection/WalletLinkConnection.ts

+
+      let connected = false;
+      switch (state) {
+        case ConnectionState.DISCONNECTED:
          // if DISCONNECTED and not destroyed


Checking if it's intentional we don't skip the initial disconnect event. Potential risk of calling ws.connect() too many times?

for the record:
with rxjs subject, it used to emit a default value DISCONNECTED whenever the observable was subscribed
by replacing it with callback listener, we expect connectionStateListener only gets invoked when there’s actual state changes. so we don’t need skip(1) for DISCONNECTED

hieronymus777 · 2023-10-27T17:43:22Z

packages/wallet-sdk/src/connection/WalletLinkConnection.ts

+            await this.authenticate();
+            this.sendIsLinked();
+            this.sendGetSessionConfig();
+            connected = true;


~~Previously we would do 'distinctUntilChanged' - meaning we would only emit connected if it changes. So we wouldn't emit two true events in a row. Just making sure this doesn't have a side effect?~~ Nevermind, I see this.connected = connected is below and correctly checks for distinct until changed.

hieronymus777 · 2023-10-27T17:44:02Z

packages/wallet-sdk/src/connection/WalletLinkConnection.ts

+          }
+          break;
+
+        case ConnectionState.CONNECTED:


We were skipping 2 events before authenticating. Not sure why this was here before but something to verify -are we sure we're not calling authenticate twice?

for ConnectionState.CONNECTED check which used to have skip(2) , i believe it was to ignore initial DISCONNECTED and CONNECTING states as the comment said. we don’t need this skipping anymore for this code block as we moved to simple switch syntax instead of individual rx subscription.

hieronymus777 · 2023-10-27T17:48:25Z

packages/wallet-sdk/src/connection/WalletLinkConnection.ts

@@ -246,15 +219,14 @@ export class WalletLinkConnection {
    this.diagnostic?.log(EVENTS.STARTED_CONNECTING, {
      sessionIdHash: Session.hash(this.sessionId),
    });
-    this.ws.connect().subscribe();
+    this.ws.connect();
  }

  /**
   * Terminate connection, and mark as destroyed. To reconnect, create a new
   * instance of WalletSDKConnection
   */
  public destroy(): void {


We were calling unsubscribe before which would clear the previous listeners. I don't see that happening anymore. Is there a possibility that the listeners will now get called after the connection is destroyed, for example through a race condition?

* [WALL-25371] Remove RxJS WalletLinkConnection * authenticate * onceConnected onceLinked * remove connectionStateSubject * adding delay function to replace rxjs delay * applying ws.connectionState$ changes * remove delay func * comment * this.connected * replace this.onceConnected$ * replace linkedSubject * cleanup * refactor RxWebSocket class * fix tests * remove rxjs from test files * cleanup * ws.setIncomingDataListener * keep markUnseenEventsAsSeen as it used to be * implement request resolve mapping * fix tests * rename RxWebSocket -> WalletLinkWebSocket * Revert "rename RxWebSocket -> WalletLinkWebSocket" This reverts commit c3ae669. * rename only the class * remove rxjs package * duh * cleanup * reimplement this.linked update pipeline * separate out once***ed logic * cleanup * this.shouldFetchUnseenEventsOnConnect * cleanup

bangtoven added 25 commits August 29, 2023 15:17

[WALL-25371] Remove RxJS WalletLinkConnection

e37d45c

authenticate

3591018

onceConnected onceLinked

c96780b

remove connectionStateSubject

56a9d24

adding delay function to replace rxjs delay

b064154

Merge branch 'master' into jungho/wlconnection-rxjs-removal

f737dc1

applying ws.connectionState$ changes

8772f2b

remove delay func

977d9b0

comment

7059e6d

this.connected

8842072

replace this.onceConnected$

0b617f1

replace linkedSubject

f25dd5c

cleanup

cfcb442

refactor RxWebSocket class

0415478

fix tests

75edd5f

remove rxjs from test files

f721f79

cleanup

9e261b7

ws.setIncomingDataListener

922fb3b

keep markUnseenEventsAsSeen as it used to be

0cf8e91

implement request resolve mapping

03a3302

fix tests

f3e61a5

rename RxWebSocket -> WalletLinkWebSocket

c3ae669

Revert "rename RxWebSocket -> WalletLinkWebSocket"

98770c7

This reverts commit c3ae669.

rename only the class

97ae87e

remove rxjs package

2e6390f

bangtoven marked this pull request as draft September 29, 2023 18:56

bangtoven requested review from vishnumad, fan-zhang-sv, erin-at-work and cb-jake September 29, 2023 18:57

duh

57cc8f2

cb-jake reviewed Sep 29, 2023

View reviewed changes

packages/wallet-sdk/src/connection/RxWebSocket.ts Outdated Show resolved Hide resolved

cb-jake reviewed Sep 29, 2023

View reviewed changes

packages/wallet-sdk/src/connection/WalletLinkConnection.ts Outdated Show resolved Hide resolved

cb-jake reviewed Sep 29, 2023

View reviewed changes

packages/wallet-sdk/src/connection/WalletLinkConnection.ts Outdated Show resolved Hide resolved

bangtoven added 4 commits October 4, 2023 12:00

cleanup

c668916

reimplement this.linked update pipeline

8f5cd02

separate out once***ed logic

8195dad

cleanup

6057932

bangtoven marked this pull request as ready for review October 6, 2023 23:28

bangtoven requested a review from cb-jake October 6, 2023 23:28

bangtoven commented Oct 6, 2023

View reviewed changes

this.shouldFetchUnseenEventsOnConnect

7ce57a7

bangtoven force-pushed the jungho/wlconnection-rxjs-removal branch from a50f5cf to 7ce57a7 Compare October 6, 2023 23:48

cleanup

d438aed

bangtoven force-pushed the jungho/wlconnection-rxjs-removal branch from 5a5c823 to d438aed Compare October 6, 2023 23:52

cb-jake reviewed Oct 10, 2023

View reviewed changes

bangtoven requested a review from hieronymus777 October 11, 2023 16:05

vishnumad approved these changes Oct 11, 2023

View reviewed changes

bangtoven merged commit a757aa5 into master Oct 11, 2023
12 checks passed

bangtoven deleted the jungho/wlconnection-rxjs-removal branch October 11, 2023 20:01

bangtoven mentioned this pull request Oct 11, 2023

Rename RxWebSocket to WalletLinkWebSocket #1026

Merged

hieronymus777 reviewed Oct 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WALL-25371] Remove RxJS from WalletLinkConnection #1018

[WALL-25371] Remove RxJS from WalletLinkConnection #1018

bangtoven commented Sep 29, 2023 •

edited

Loading

cb-jake Sep 29, 2023

bangtoven Oct 4, 2023

cb-jake commented Sep 29, 2023

bangtoven Oct 6, 2023

cb-jake Oct 10, 2023

bangtoven Oct 10, 2023

cb-jake Oct 10, 2023

cb-jake left a comment

hieronymus777 Oct 27, 2023 •

edited

Loading

bangtoven Nov 14, 2023

hieronymus777 Oct 27, 2023 •

edited

Loading

hieronymus777 Oct 27, 2023

bangtoven Nov 14, 2023

hieronymus777 Oct 27, 2023

bangtoven Oct 27, 2023


		import { ConnectionState, RxWebSocket } from './RxWebSocket';
		import { ConnectionState, WalletLinkWebSocket } from './RxWebSocket';

[WALL-25371] Remove RxJS from WalletLinkConnection #1018

[WALL-25371] Remove RxJS from WalletLinkConnection #1018

Conversation

bangtoven commented Sep 29, 2023 • edited Loading

Summary

How did you test your changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cb-jake commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cb-jake left a comment

Choose a reason for hiding this comment

hieronymus777 Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hieronymus777 Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangtoven commented Sep 29, 2023 •

edited

Loading

hieronymus777 Oct 27, 2023 •

edited

Loading

hieronymus777 Oct 27, 2023 •

edited

Loading