Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] Update max number of listeners on stateChange event for CallAdapter #5660

Merged
merged 4 commits into from
Feb 21, 2025

Conversation

dmceachernmsft
Copy link
Member

@dmceachernmsft dmceachernmsft commented Feb 21, 2025

What

Updates the max number of event listeners that can be applied to an event.

Why

The warning that we were seeing was sepcifically caused by the max number of event subscriptions on the 'stateChanged' event for the adapter. This was previously set to 50 as the max.

Recently we started making changes to how we were getting data from the state of the adapter by writing selectors for all of the data that we would be retrieving. This is what sent us over the limit. when we call useSelector like so
image
this under the hood calls a special hook for the adapter called useAdaptedSelector. in this hook we have a useEffect defined as follows
image
for every render that would go by all of the different selectors would unmount, removing listeners from the event. Then when the components of the composite would all go requesting their data again if the state changed causing the count of subscriptions to go up.

From testing the cap that we would see for the listeners on this event would be just over 100. ivestigating into the usage of useSelector in the CallComposite the number is 94 usages of the hook.
image
Since Typescript is linear we would see subscriptions to the event from different selectors around the composite before they were all cleaned up so we see the number of subscriptions going a little higher than 100 but they are always cleaned up down to a much smaller number.

Suggesting we bump the number of subscriptions here to give us a little more room to write more selectors in the CallComposite without causing the warning to come back.

https://skype.visualstudio.com/SPOOL/_workitems/edit/3964503

How Tested

Validated and investigated locally

Copy link
Contributor

Copy link
Contributor

github-actions bot commented Feb 21, 2025

@azure/communication-react jest test coverage for stable.

Lines Statements Functions Branches
Base 28010 / 44676
62.69%
28010 / 44676
62.69%
786 / 1436
54.73%
2353 / 3720
63.25%
Current 28005 / 44676
62.68%
28005 / 44676
62.68%
786 / 1436
54.73%
2365 / 3727
63.45%
Diff -5 / 0
-0.01%
-5 / 0
-0.01%
0 / 0
0%
12 / 7
0.2%

Copy link
Contributor

github-actions bot commented Feb 21, 2025

@azure/communication-react jest test coverage for beta.

Lines Statements Functions Branches
Base 58244 / 94212
61.82%
58244 / 94212
61.82%
1177 / 2693
43.7%
3567 / 5842
61.05%
Current 58246 / 94221
61.81%
58246 / 94221
61.81%
1177 / 2693
43.7%
3527 / 5818
60.62%
Diff 2 / 9
-0.01%
2 / 9
-0.01%
0 / 0
0%
-40 / -24
-0.43%

Copy link
Contributor

Chat bundle size is not changed.

  • Current size: 1777281
  • Base size: 1777281
  • Diff size: 0

Copy link
Contributor

CallWithChat bundle size is not changed.

  • Current size: 12401112
  • Base size: 12401112
  • Diff size: 0

Copy link
Contributor

Calling bundle size is not changed.

  • Current size: 12401100
  • Base size: 12401100
  • Diff size: 0

Copy link
Contributor

@dmceachernmsft dmceachernmsft merged commit 998514c into main Feb 21, 2025
41 checks passed
@dmceachernmsft dmceachernmsft deleted the dmceachernmsft/event-warning branch February 21, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants