Refactor CallCapabilities #1170

jpsantosbh · 2025-02-05T14:11:48Z

Description

This

exports all the CallCapabilities interfaces
changes the flag type from true | undefined => boolean
use class objects to handle the payload mapping

Type of change

Internal refactoring
Bug fix (bugfix - non-breaking)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

In case of new feature or breaking changes, please include code snippets.

changeset-bot · 2025-02-05T14:11:52Z

⚠️ No Changeset found

Latest commit: 1324791

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

iAmmar7 · 2025-02-05T14:23:33Z

@jpsantosbh could you please check the CI?

Also, the event emits the capabilities from here: https://github.com/signalwire/signalwire-js/blob/main/packages/js/src/fabric/workers/callJoinWorker.ts#L83

The TS interface does not understand it since the server returns an array of strings. That is why the consumer has been facing the TS mismatch issue. We need to find out a way to fix the TS interface for the event.

iAmmar7 · 2025-02-06T16:01:11Z

packages/core/src/types/fabricRoomSession.ts

@@ -506,7 +507,7 @@ export interface CallJoinedEventParams {
  member_id: string
  node_id?: string
  origin_call_id: string
-  capabilities: string[] // TODO: More stronger type is required through server
+  capabilities: CallCapabilities


This is the interface for the server event. The type of the capabilities on the server event is an array of string.

We don't 2 distinct interfaces one "from the server" and another "to emit".
Since we want SDK exposed events to have the interface we're emitting we need to define it here.

The interfaces you see within this types folder are for the events from the server. We used the same interface for both (receiving and emitting) since what we receive is exactly what we emit.

Now, only because of this capability change, we need to create two interfaces. One for the event received by the server, the other for the event emitted by the SDK.

iAmmar7 · 2025-02-06T16:07:02Z

packages/js/src/fabric/workers/callJoinWorker.ts

@@ -74,10 +74,10 @@ export const callJoinWorker = function* (
  })

  cfRoomSession.member = get<FabricRoomSessionMember>(payload.member_id)
-  cfRoomSession.capabilities = mapCapabilityPayload(payload.capabilities || [])
+  // the server send the capabilities payload as an array of string 
+  cfRoomSession.capabilities = mapCapabilityPayload(payload.capabilities as unknown as string[] || [])


The above mentioned interface is the issue why we need to use this "as unknown as string[]" here.

yes, but when that is defined as string[], then we get a type error in the emit call.
since the two payloads are different, IMO the SDK should expose the type for the mapped payload,
while here we just do the cast intentionally (that's why I added the comment)

You do not need to do type casting if we have two different interfaces, one for receiving and the other for emitting.

With this change right now, we have broken the server event interface.

packages/core/src/utils/eventUtils.ts

packages/core/src/types/fabric.ts

iAmmar7 · 2025-02-06T16:16:53Z

packages/core/src/types/fabric.ts

-  screenshare?: true
+  self: MemberCapability
+  member: MemberCapability
+  end: boolean


This seems to be not present in the docs, probably a typo?

packages/core/src/types/fabric.ts

iAmmar7

I do not think we should be adding an unnecessary inconsistency in the SDK. Some of the capabilities are not inconsistent with what we get from the server.

It will become harder to debug and understand which capability (from the server) is translated to which capability (by the SDK).

I would suggest either of the below options:

Expose a well defined array of string with proper TS. In the FabricRoomSession class, we can convert that into a JS Set so that the retrieval is log(1) - constant.
Expose object with the same path as the server string.

I think it's easier to do the step 1, if there is no real benefit of the option 2.

jpsantosbh · 2025-02-06T20:59:53Z

I do not think we should be adding an unnecessary inconsistency in the SDK. Some of the capabilities are not inconsistent with what we get from the server.

It will become harder to debug and understand which capability (from the server) is translated to which capability (by the SDK).

I would suggest either of the below options:

Expose a well defined array of string with proper TS. In the FabricRoomSession class, we can convert that into a JS Set so that the retrieval is log(1) - constant.

Expose object with the same path as the server string.

I think it's easier to do the step 1, if there is no real benefit of the option 2.

Honestly, I don't like both suggestions.(We discussed this previously when the feature was introduced in the SDK)

This PR is about fixing the emitted payload type and changing from true|undefined => boolean.
It already passed the unit test using the same interface the app already used on its implementation, and I don't want to break the app or do a new unit test for something that is working.

iAmmar7 · 2025-02-07T12:26:42Z

I discussed it with the consumer, and the consumer is fine with the object. So, I guess it's fine to have it. I value more consumer feedback. So, apologies, if I caused any confusion, let's target creating a new interface for the SDK emit and merge this PR.

You may create the event interface in this file and then update the FabricRoomSessionEventsHandlerMap: https://github.com/signalwire/signalwire-js/blob/main/packages/js/src/utils/interfaces/fabric.ts

For the server capability string to SDK object, maybe we can write some documents later on. So that, we remember these changes.

iAmmar7

Overall looks good to me. These are minor suggestions for TS interface and types export.

iAmmar7 · 2025-02-10T17:26:28Z

packages/core/src/types/fabricRoomSession.ts

@@ -506,7 +506,7 @@ export interface CallJoinedEventParams {
  member_id: string
  node_id?: string
  origin_call_id: string
-  capabilities: string[] // TODO: More stronger type is required through server
+  capabilities: string[]


Can you please add back this todo? So that we remember to update this later.

iAmmar7 · 2025-02-10T17:28:37Z

packages/core/src/types/fabric.ts

-interface CapabilityOnOffState {
-  on?: true
-  off?: true
+export interface CapabilityOnOffState {


Do you mind moving this to the js/src/fabric folder since it's only required in Fabric and is not associated with any SDK event?

We can probably create a new file here packages/js/src/fabric/interfaces. We also need to export the CallCapabilities type from the JS package.

iAmmar7 · 2025-02-10T17:30:41Z

packages/core/src/utils/eventUtils.ts

-  'self.raisehand.off': {
-    self: { raisehand: { off: true } },
-  } as CallCapabilities,
+class CapabilityOnOffStateImpl implements CapabilityOnOffState {


This thing can also be moved to a dedicated CallCapabilities file inside the js/src/fabric.

iAmmar7 · 2025-02-10T17:33:40Z

packages/core/src/utils/eventUtils.ts

+  }
+}
+
+class MemberCapabilityImpl implements MemberCapability {


Instead of using the suffix "Impl", you can create a class with the normal name such as "MemberCapability" and use the suffix "Contract" for the TS interface such as "MemberCapabilityContract".

That's what the SDK does with other classes.

iAmmar7 · 2025-02-10T17:34:58Z

packages/core/src/utils/eventUtils.ts

+    return this._flags.some((flag) =>
+      /^(.*\.on|(?:(?!.*\.off$).*))$/.test(flag)
+    )


Can this be updated into simpler JS?

Suggested change

return this._flags.some((flag) =>

/^(.*\.on|(?:(?!.*\.off$).*))$/.test(flag)

)

return this._flags.some(flag => !flag.endsWith('.off'));

iAmmar7 · 2025-02-11T12:37:25Z

packages/js/src/fabric/FabricRoomSession.ts

+        ? !this.capabilities?.self?.muteAudio?.off
+        : !this.capabilities?.member?.muteAudio?.off


Once we have the list of capabilities, the SDK makes sure to include all the properties, right?

Then we can update this a bit:

Suggested change

? !this.capabilities?.self?.muteAudio?.off

: !this.capabilities?.member?.muteAudio?.off

? !this.capabilities?.self.muteAudio.off

: !this.capabilities?.member.muteAudio.off

iAmmar7 · 2025-02-11T12:40:32Z

packages/js/src/fabric/workers/callJoinWorker.ts

-  // FIXME: Capabilities type is incompatible.
-  // @ts-expect-error
-  cfRoomSession.emit('call.joined', {
+  const fabricEvent: FabricCallJoinedEventParams = {


We do not need type casting anymore:

Suggested change

const fabricEvent: FabricCallJoinedEventParams = {

const fabricEvent = {

iAmmar7 · 2025-02-11T12:40:45Z

packages/js/src/fabric/workers/callJoinWorker.ts

-    capabilities: cfRoomSession.capabilities,
-  })
+    capabilities: cfRoomSession.capabilities
+  } as FabricCallJoinedEventParams


We do not need type casting anymore:

Suggested change

} as FabricCallJoinedEventParams

}

Without it, the result will have capabilities unknown.
This is a TS, I believe, because we are spreading the payload and in there capabilities have a distinct type.

Interesting, I actually tried this again, and it seems to be not throwing any error.

const fabricEvent = { ...payload, capabilities: cfRoomSession.capabilities, } cfRoomSession.emit('call.joined', fabricEvent)

Maybe we can connect over the call and go through this if required. Just make sure you are re-running the TS; sometimes it shows error, and re-running solves it.

iAmmar7 · 2025-02-11T12:42:20Z

packages/js/src/utils/interfaces/fabric.ts

@@ -73,6 +74,11 @@ export type FabricMemberListUpdatedParams = {
  members: InternalFabricMemberEntity[]
 }

+export type FabricCallJoinedEventParams =  {
+  capabilities: CallCapabilities
+} & Omit<CallJoinedEventParams, 'capabilities'>


Maybe later on, we will need to use some standard names. Maybe prefix it with "Internal" that are only consumed by the SDK not available for the public. For now it's fine though.

iAmmar7 · 2025-02-11T12:45:43Z

packages/js/src/utils/interfaces/fabric.ts

@@ -73,6 +74,11 @@ export type FabricMemberListUpdatedParams = {
  members: InternalFabricMemberEntity[]
 }

+export type FabricCallJoinedEventParams =  {


We need to export this from here as well: packages/js/src/fabric/interfaces/index.ts, so that consumer can access this from the Fabric SDK.

Same for CallCapabilities types.

refactor CallCapabilities

061eed9

jpsantosbh requested a review from iAmmar7 February 5, 2025 14:11

jpsantosbh added 3 commits February 5, 2025 11:23

fix

db6ebb7

emit with correct type

c3db9b9

bug fixes

5974171

iAmmar7 reviewed Feb 6, 2025

View reviewed changes

packages/core/src/utils/eventUtils.ts Show resolved Hide resolved

iAmmar7 reviewed Feb 6, 2025

View reviewed changes

packages/core/src/types/fabric.ts Show resolved Hide resolved

iAmmar7 reviewed Feb 6, 2025

View reviewed changes

packages/core/src/types/fabric.ts Show resolved Hide resolved

iAmmar7 reviewed Feb 6, 2025

View reviewed changes

packages/core/src/types/fabric.ts Show resolved Hide resolved

iAmmar7 requested changes Feb 6, 2025

View reviewed changes

server and fabric types

5575127

jpsantosbh requested a review from iAmmar7 February 10, 2025 16:48

iAmmar7 reviewed Feb 11, 2025

View reviewed changes

expose

1324791

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CallCapabilities #1170

Refactor CallCapabilities #1170

jpsantosbh commented Feb 5, 2025

changeset-bot bot commented Feb 5, 2025 •

edited

Loading

iAmmar7 commented Feb 5, 2025

iAmmar7 Feb 6, 2025

jpsantosbh Feb 6, 2025

iAmmar7 Feb 7, 2025

iAmmar7 Feb 6, 2025

jpsantosbh Feb 6, 2025

iAmmar7 Feb 7, 2025

iAmmar7 Feb 6, 2025

iAmmar7 left a comment •

edited

Loading

jpsantosbh commented Feb 6, 2025

iAmmar7 commented Feb 7, 2025

iAmmar7 left a comment

iAmmar7 Feb 10, 2025

iAmmar7 Feb 10, 2025

iAmmar7 Feb 10, 2025

iAmmar7 Feb 10, 2025

iAmmar7 Feb 10, 2025

iAmmar7 Feb 11, 2025

iAmmar7 Feb 11, 2025

iAmmar7 Feb 11, 2025

jpsantosbh Feb 11, 2025

iAmmar7 Feb 11, 2025

iAmmar7 Feb 11, 2025

iAmmar7 Feb 11, 2025

		? !this.capabilities?.self?.muteAudio?.off
		: !this.capabilities?.member?.muteAudio?.off

	const fabricEvent: FabricCallJoinedEventParams = {
	const fabricEvent = {

Refactor CallCapabilities #1170

Are you sure you want to change the base?

Refactor CallCapabilities #1170

Conversation

jpsantosbh commented Feb 5, 2025

Description

Type of change

Code snippets

changeset-bot bot commented Feb 5, 2025 • edited Loading

⚠️ No Changeset found

iAmmar7 commented Feb 5, 2025

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

iAmmar7 left a comment • edited Loading

Choose a reason for hiding this comment

jpsantosbh commented Feb 6, 2025

iAmmar7 commented Feb 7, 2025

iAmmar7 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

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

changeset-bot bot commented Feb 5, 2025 •

edited

Loading

iAmmar7 left a comment •

edited

Loading