refactor(plugin-meetings): update tsconfig for plugin-meetings #3663

antsukanova · 2024-06-25T13:32:53Z

COMPLETES WEBEX-390013

This pull request addresses

The plugin-meetings previously had weak tsconfig rules. The purpose of this PR is to improve the tsconfig rules to help identify issues faster with the data across this plugin. I am looking forward to working together with you to enhance the SDK code.

Any suggestions are welcome.

In this PR, I do not intend to fix all the types, but rather enable better handling of types.

by making the following changes

I would like to explain some of my refactoring ideas here:

I have used the correct types wherever possible, and I have used unknown for types that I am not certain about. In cases where there is an object, you may see Record<string, any>, and there were a couple of instances with any that were difficult to fix easily because they required changes in the codebase.
Additionally, I have added a global.d.ts file to mock types from other plugins. The idea is to assist TypeScript in working correctly for now, and once these plugins are changed, these declarations should be deleted along with the file.

Change Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Tooling change
Internal code refactor

The following scenarios where tested

automated tests for plugin-meetings

I certified that

I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly

Make sure to have followed the contributing guidelines before submitting.

aws-amplify-us-east-2 · 2024-06-25T13:43:53Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3663.d3m3l2kee0btzx.amplifyapp.com

antsukanova · 2024-06-25T15:44:20Z

packages/@webex/plugin-meetings/src/breakouts/index.ts

@@ -479,7 +496,7 @@ const Breakouts = WebexPlugin.extend({
            locusUrl: this.locusUrl,
          },
        })
-        .catch((err) => {
+        .catch((err: unknown) => {


I would like to set unknown everywhere in this PR, but there are cases where err is explicitly an object. With object handling, the only way to handle it is to set it to any due to the lack of error handlers for errors (see screenshot). In the future, it would be great to create a proper Error type structure and align it accordingly.

antsukanova · 2024-06-25T15:50:34Z

packages/@webex/plugin-meetings/src/breakouts/request.ts

@@ -25,7 +25,7 @@ export default class BreakoutRequest extends StatelessWebexPlugin {
  }: {
    url: string;
    message: string;
-    options?: object;


Ideally, it would be to avoid object, as it's less flexible than Record<string, any> in short. Here is some discussion about that: https://stackoverflow.com/a/66575880

antsukanova · 2024-06-25T15:59:33Z

packages/@webex/plugin-meetings/src/common/queue.ts

    if (this.queue.length === 0) {
      return null;
    }

-    return this.queue.shift();
+    return this.queue.shift() ?? null;


Array<ItemType>.shift(): ItemType | undefined, so add here null to align logic

antsukanova · 2024-06-25T16:33:53Z

packages/@webex/plugin-meetings/src/locus-info/parser.ts

-   * @returns {undefined}
-   */
-  // eslint-disable-next-line @typescript-eslint/no-unused-vars
-  onDeltaAction(action: string, locus) {}


Just added the proper type, and this won't be needed.

antsukanova · 2024-06-25T16:40:09Z

packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts

-SelfUtils.getBreakoutSessions = (self) => self?.controls?.breakout?.sessions;
-SelfUtils.getBreakout = (self) => self?.controls?.breakout;
-SelfUtils.getInterpretation = (self) => self?.controls?.interpretation;
+SelfUtils.getBreakoutSessions = (self: Record<string, any>) => self?.controls?.breakout?.sessions;


Btw, I already have a branch that I will add to review later for updating with the proper types of locus utils.

antsukanova · 2024-06-25T16:42:50Z

packages/@webex/plugin-meetings/src/media/index.ts

@@ -222,15 +222,16 @@ Media.createMediaConnection = (
        audio: audioStream?.outputStream?.getTracks()[0],
        video: videoStream?.outputStream?.getTracks()[0],
        screenShareVideo: shareVideoStream?.outputStream?.getTracks()[0],
+        // @ts-ignore
        screenShareAudio: shareAudioStream?.outputStream?.getTracks()[0], // TODO: add type for screenShareAudio in internal-media-core SPARK-446923


For this issue, I have to add @ts-ignore because otherwise, types will not match. I see there is already a TODO for this issue, so I hope it will be fixed soon.

antsukanova · 2024-06-26T08:37:23Z

packages/@webex/plugin-meetings/src/multistream/mediaRequestManager.ts

+    const maxFps = mediaRequest.codecInfo?.maxFps || CODEC_DEFAULTS.h264.maxFps;
+    const maxFs = mediaRequest.codecInfo?.maxFs || CODEC_DEFAULTS.h264.maxFs;


From types it seems that both maxFps and maxFs can be undefined, so I added here default value too

the comment explains why in practice maxFs cannot be undefined, but it doesn't hurt to have the fallback

antsukanova · 2024-06-26T08:39:17Z

packages/@webex/plugin-meetings/src/multistream/remoteMediaManager.ts

@@ -1161,7 +1161,8 @@ export class RemoteMediaManager extends EventsScope {
    remoteMedia.stop();

    delete this.media.video.memberPanes[paneId];
-    delete this.currentLayout.memberVideoPanes?.[paneId];
+    // TODO: to delete memberVideoPanes using paneId it needs to be a number


I put todo here because types for paneId does not match and need to be changed properly

this looks like a real bug, the paneId is always a string and memberVideoPanes is an array of MemberVideoPane, so instead of this delete we should be finding the pane by paneId in the memberVideoPanes and removing it, something like this:
this.currentLayout.memberVideoPanes = this.currentLayout.memberVideoPanes.filter((pane) => pane.id !== paneId))

also the check if (newPane.id in this.currentLayout.memberVideoPanes) inside addMemberVideoPane() seems wrong too. Can you raise a jira for this, please?

@marcin-bazyl sure

antsukanova · 2024-06-26T08:41:53Z

packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts

@@ -70,7 +70,7 @@ export class ClusterReachability {
   * @returns {Number} Milliseconds
   */
  private getElapsedTime() {
-    return Math.round(performance.now() - this.startTimestamp);
+    return Math.round(performance.now() - (this.startTimestamp || 0));


this.startTimestamp wasn't initiated, so the type tells me that it can be undefined. I put a fallback to make types not complain about that.

this function should probably throw if it's called before this.startTimestamp is set, as this should never happen

antsukanova · 2024-06-26T08:44:13Z

packages/@webex/plugin-meetings/src/reachability/index.ts

-  private getUnreachableClusters(): Array<{name: string; protocol: string}> {
-    const unreachableList = [];
+  private getUnreachableClusters(): {name: string; protocol: string}[] {
+    const unreachableList: {name: string; protocol: string}[] = [];


Just to align types, I used the more commonly used type definition for arrays [] everywhere I could and suggest preferring one style for the entire codebase.

can this be enforced automatically with eslint? otherwise it will keep coming up

BTW, personally I prefer to use the Array<> syntax if the elements of the array have a long complicated type like an object with multiple properties, because it's easier to miss the [] at the end when reading it, but I don't mind which syntax we enforce.

antsukanova · 2024-06-26T08:48:46Z

packages/@webex/plugin-meetings/tsconfig.json

@@ -1,6 +1,41 @@
 {
    "extends": "../../../tsconfig.json",
+    "compilerOptions": {
+      "module": "es2020",


I combined rules from the calling plugin and also added some useful type rules from WCME. I'm open to discussing any details about these rules, as I might have missed some crucial points that are necessary for the SDK. I appreciate your effort in asking about this.

antsukanova · 2024-06-26T08:51:33Z

packages/webex/package.json

@@ -71,9 +71,14 @@
    "@webex/common": "workspace:*",
    "@webex/internal-plugin-calendar": "workspace:*",
    "@webex/internal-plugin-device": "workspace:*",
+    "@webex/internal-plugin-dss": "workspace:*",


I'm not sure why these plugins weren't included before, but this is the reason why I have an issue with the test-style falling. If you know specific reasons for not including it, please let me know, and we can try to find other ways to fix that. Thanks!

marcin-bazyl · 2024-06-25T13:51:42Z

packages/@webex/plugin-meetings/src/global.d.ts

+
+  export = AmpCollection;
+}
+declare module 'javascript-state-machine' {


maybe for javascript-state-machine we could use https://www.npmjs.com/package/@types/javascript-state-machine?

Thanks, good point. I will add it. It's worth doing now.

marcin-bazyl · 2024-06-25T15:14:46Z

packages/@webex/plugin-meetings/src/meeting/in-meeting-actions.ts

-  supportHDV?: boolean;
-  canShareWhiteBoard?: boolean;
-  enforceVirtualBackground?: boolean;
+  canInviteNewParticipants?: boolean | null;


for all these fields - they already have the ?: in the type declaration so they can be undefined, so maybe instead of adding | null it would be better to just initialise them to undefined in the constructor?

@marcin-bazyl Good point, yes. I saw a couple of files that just define it like that. I wasn't sure if it was okay to change, so I've made a soft fix. I will update this approach then. Thanks

antsukanova self-assigned this Jun 25, 2024

antsukanova marked this pull request as ready for review June 25, 2024 13:32

antsukanova requested review from a team as code owners June 25, 2024 13:32

Shreyas281299 added the validated If the pull request is validated for automation. label Jun 25, 2024

Anna Tsukanova added 7 commits June 25, 2024 17:20

fix(plugin-meetings): rebase next

17376c8

fix(plugin-meetings): update tsconfig

383c3ec

refactor(plugin-meetings): refactor tsconfig

2521c83

refactor(plugin-meetings): fix tests

4c351cf

refactor(plugin-meetings): fix global.d.ts

d0893f3

refactor(plugin-meetings): fix type issue after rebasing

87ef47b

refactor(plugin-meetings): fix test style tests

e9a1a00

antsukanova force-pushed the antsukan/ts-config branch from 01fa594 to e9a1a00 Compare June 25, 2024 15:21

antsukanova commented Jun 25, 2024

View reviewed changes

refactor(plugin-meetings): updata types

32dd8d7

antsukanova commented Jun 25, 2024

View reviewed changes

refactor(plugin-meetings): updata types

d34177c

antsukanova commented Jun 25, 2024

View reviewed changes

refactor(plugin-meetings): updata parser

4eb71c2

antsukanova commented Jun 25, 2024

View reviewed changes

refactor(plugin-meetings): fix type declarations

40996a6

antsukanova commented Jun 26, 2024

View reviewed changes

refactor(plugin-meetings): fix type declarations

9434b53

marcin-bazyl reviewed Jun 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(plugin-meetings): update tsconfig for plugin-meetings #3663

refactor(plugin-meetings): update tsconfig for plugin-meetings #3663

antsukanova commented Jun 25, 2024 •

edited

Loading

aws-amplify-us-east-2 bot commented Jun 25, 2024

antsukanova Jun 25, 2024

antsukanova Jun 25, 2024

antsukanova Jun 25, 2024

antsukanova Jun 25, 2024 •

edited

Loading

antsukanova Jun 25, 2024 •

edited

Loading

antsukanova Jun 25, 2024

antsukanova Jun 26, 2024

marcin-bazyl Jun 26, 2024

antsukanova Jun 26, 2024

marcin-bazyl Jun 26, 2024

antsukanova Jun 26, 2024

antsukanova Jun 26, 2024

marcin-bazyl Jun 27, 2024

antsukanova Jun 26, 2024

marcin-bazyl Jun 27, 2024

antsukanova Jun 26, 2024

antsukanova Jun 26, 2024

marcin-bazyl Jun 25, 2024

antsukanova Jun 26, 2024

marcin-bazyl Jun 25, 2024

antsukanova Jun 26, 2024

		const maxFps = mediaRequest.codecInfo?.maxFps \|\| CODEC_DEFAULTS.h264.maxFps;
		const maxFs = mediaRequest.codecInfo?.maxFs \|\| CODEC_DEFAULTS.h264.maxFs;

refactor(plugin-meetings): update tsconfig for plugin-meetings #3663

Are you sure you want to change the base?

refactor(plugin-meetings): update tsconfig for plugin-meetings #3663

Conversation

antsukanova commented Jun 25, 2024 • edited Loading

COMPLETES WEBEX-390013

This pull request addresses

by making the following changes

Change Type

The following scenarios where tested

I certified that

aws-amplify-us-east-2 bot commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsukanova Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

antsukanova Jun 25, 2024 • 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

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

antsukanova commented Jun 25, 2024 •

edited

Loading

antsukanova Jun 25, 2024 •

edited

Loading

antsukanova Jun 25, 2024 •

edited

Loading