feat: add custom method toJSON for _LocalStream #70

antsukanova · 2024-01-23T19:47:25Z

The main purpose of this update is to make better work with LocalStream and smoother debugging for webex-js-sdk. Mainly it should get rid of custom overrides for objects and give more useful information in better structure. See current overrides here webex/webex-js-sdk@4089648

This PR consists of the following updates:

update for _LocalStream object and add custom toJSON method
added new test cases for _LocalStream

brycetham

Code looks fine, but I'll defer to @marcin-bazyl to see if the output of the toJSON function matches his expectations. I noticed the current workaround in the SDK includes label, readyState, and enabled (though I'd argue we should use muted instead for the last one).

brycetham · 2024-01-23T20:30:48Z

src/media/local-stream.ts

@@ -283,6 +283,32 @@ abstract class _LocalStream extends Stream {
    return this.effects;
  }

+  /**
+   * Method to serialize data about input, output streams
+   * and also effects from _LocalStream.


Might want to remove the "_" from your comments. Users of webrtc-core will only ever see LocalStream, never _LocalStream.

bbaldino · 2024-01-23T23:54:50Z

src/media/local-stream.ts

+   *
+   * @returns - A JSON-compatible object representation with data from _LocalStream.
+   */
+  toJSON() {


nit: let's do toJson

toJson won't work, it has to be toJSON, because that's what JSON.stringify() calls, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify

Oh, sorry didn't realize this was an automatic hook for JSON.stringify. 👍

bbaldino · 2024-01-23T23:55:46Z

src/media/local-stream.ts

+   * @returns - A JSON-compatible object representation with data from _LocalStream.
+   */
+  toJSON() {
+    return {


Maybe another nitpick, but this is returning an objec, not "json", right? I think if this method is going to be called toJson we should JSON.stringify and return a String.

@bbaldino yes and with your idea, it requires you to manually call your custom method each time you need it, and it won't be used automatically by JSON.stringify(). That's more transparent. But I also want to ask @marcin-bazyl will it be convenient to use it in this way?

@bbaldino the whole point of this jira is to make JSON.stringify() work, not to invent new methods. Any library/component that uses webrtc-core should be free to call JSON.stringify() on instances of webrtc-core classes like LocalStream and that should not cause the browser to hang up (that's what's currently happening).

marcin-bazyl · 2024-01-24T14:49:55Z

Code looks fine, but I'll defer to @marcin-bazyl to see if the output of the toJSON function matches his expectations. I noticed the current workaround in the SDK includes label, readyState, and enabled (though I'd argue we should use muted instead for the last one).

yeah, good point, it would be good to include label, readyState, enabled and muted from the inputTrack

antsukanova · 2024-01-25T12:42:23Z

src/media/local-stream.ts

+        id: this.inputStream.id,
+        muted: this.muted,
+        label: this.label,
+        enabled: this.inputTrack.enabled,


@bbaldino @brycetham not sure that for one case here it is useful to have a getter for enabled. So right now put it via inputTrack. Let me know if you have any other thoughts

Yeah I wouldn't add a getter for enabled unless there is a need for it outside this function. Actually, I would argue that we shouldn't include enabled here at all, since muted already takes enabled into account.

I would also probably move muted, label, and readyState out of inputStream and put it at the same level as inputStream, outputStream, and effects. These things, in my opinion, apply to the whole LocalStream, not just the input (hence why we have explicitly defined getters for all of them).

@marcin-bazyl please let me know what you think about Bryce's idea to remove the enabled state.

for debugging purposes it's better to keep both muted and enabled

Ok, but let's keep it separate from the overall LocalStream mute state. So I would suggest something like this:

{ muted: this.muted, inputStream: { muted: this.inputTrack.muted, enabled: this.inputTrack.enabled, ... }, ... }

@brycetham updated recording your advice, thanks
@bbaldino @marcin-bazyl FYI

antsukanova · 2024-01-25T12:45:27Z

@marcin-bazyl @bbaldino @brycetham here is an example from the console of how it looks like for the test-app. Please let me know @marcin-bazyl if it is what you need.

brycetham · 2024-01-26T13:59:08Z

src/media/local-stream.ts

+  toJSON() {
+    return {
+      muted: this.muted,
+      enabled: this.inputTrack.enabled,


Sorry if I wasn't clear in my previous comment. I meant let's keep enabled in inputStream and also have a separate muted property in inputStream as well. So, let's remove enabled here...

@brycetham I understand your idea now. It sounds logical. Thank you for clarifying it for me. Will update

brycetham · 2024-01-26T14:02:54Z

src/media/local-stream.ts

+      readyState: this.readyState,
+      inputStream: {
+        active: this.inputStream.active,
+        id: this.inputStream.id,


...and add enabled: this.inputTrack.enabled and muted: this.inputTrack.muted here. This way, we can tell the enabled and muted state of the input track apart from the overall muted state of the LocalStream.

@brycetham updated this part, thanks)

brycetham

Looks good to me 👍

antsukanova · 2024-01-29T09:48:41Z

Thanks, guys, for your review. Merge it 👍

# [2.4.0](v2.3.1...v2.4.0) (2024-01-29) ### Features * add custom method toJSON for _LocalStream ([#70](#70)) ([6ac5ba1](6ac5ba1))

feat: add custom toJSON for _LocalStream

ca83db4

antsukanova requested review from brycetham and bbaldino January 23, 2024 19:48

brycetham requested a review from marcin-bazyl January 23, 2024 20:20

brycetham reviewed Jan 23, 2024

View reviewed changes

bbaldino reviewed Jan 23, 2024

View reviewed changes

chore: update test and fix naming in docs

2fb0ee7

chore: update toJSON and tests

599efa1

antsukanova commented Jan 25, 2024

View reviewed changes

antsukanova requested review from bbaldino and brycetham January 25, 2024 12:45

chore: update toJSON and tests

ea74dec

brycetham reviewed Jan 26, 2024

View reviewed changes

chore: update toJSON muted and enabled state, update tests

b068ec2

antsukanova requested a review from brycetham January 26, 2024 15:27

brycetham approved these changes Jan 26, 2024

View reviewed changes

marcin-bazyl approved these changes Jan 26, 2024

View reviewed changes

antsukanova merged commit 6ac5ba1 into main Jan 29, 2024
1 check passed

antsukanova deleted the antsukan/SPARK-467164 branch January 29, 2024 09:49

antsukanova self-assigned this Jan 29, 2024

antsukanova added the enhancement New feature or request label Jan 29, 2024

bbaldino pushed a commit that referenced this pull request Jan 29, 2024

chore(release): 2.4.0 [skip ci]

4efe11e

# [2.4.0](v2.3.1...v2.4.0) (2024-01-29) ### Features * add custom method toJSON for _LocalStream ([#70](#70)) ([6ac5ba1](6ac5ba1))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add custom method toJSON for _LocalStream #70

feat: add custom method toJSON for _LocalStream #70

antsukanova commented Jan 23, 2024

brycetham left a comment

brycetham Jan 23, 2024

bbaldino Jan 23, 2024

marcin-bazyl Jan 24, 2024

bbaldino Jan 24, 2024

bbaldino Jan 23, 2024

antsukanova Jan 24, 2024

marcin-bazyl Jan 24, 2024

bbaldino Jan 24, 2024

marcin-bazyl commented Jan 24, 2024

antsukanova Jan 25, 2024

brycetham Jan 25, 2024

antsukanova Jan 25, 2024

marcin-bazyl Jan 25, 2024

brycetham Jan 25, 2024

antsukanova Jan 25, 2024

antsukanova commented Jan 25, 2024

brycetham Jan 26, 2024

antsukanova Jan 26, 2024

brycetham Jan 26, 2024

antsukanova Jan 26, 2024

brycetham left a comment

antsukanova commented Jan 29, 2024

feat: add custom method toJSON for _LocalStream #70

feat: add custom method toJSON for _LocalStream #70

Conversation

antsukanova commented Jan 23, 2024

brycetham left a 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

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

marcin-bazyl commented Jan 24, 2024

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

Choose a reason for hiding this comment

antsukanova commented Jan 25, 2024

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

brycetham left a comment

Choose a reason for hiding this comment

antsukanova commented Jan 29, 2024