Enable noUncheckedIndexedAccess in StatefulCallClient and Calling-Component-Bindings#5242
Merged
JamesBurnside merged 6 commits intomainfrom Oct 4, 2024
Merged
Conversation
JamesBurnside
requested review from
JoshuaLai,
Leah-Xia-Microsoft,
PorterNan,
alkwa-msft,
carocao-msft,
dmceachernmsft,
edwardlee-msft,
emlynmac,
jimchou-dev,
jpeng-ms,
mgamis-msft,
palatter,
prabhjot-msft,
vhuseinova-msft and
waynemo
Contributor
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-mbvxkxusft.chromatic.com/ |
Contributor
Chat bundle size is not changed.
|
Contributor
Calling bundle size is not changed.
|
Contributor
CallWithChat bundle size is not changed.
|
Contributor
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-micusmzsst.chromatic.com/ |
Contributor
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-pxnizdoxcz.chromatic.com/ |
Contributor
@azure/communication-react jest test coverage for stable.
|
Contributor
@azure/communication-react jest test coverage for beta.
|
dmceachernmsft
approved these changes
Oct 3, 2024
| expect(Object.keys(context.getState().callsEnded).length).toBe(1); | ||
| expect(context.getState().callsEnded[mockCallId].callEndReason?.code).toBe(1); | ||
| expect(context.getState().callsEnded[mockCallId].endTime).toBeTruthy(); | ||
| expect(context.getState().callsEnded[mockCallId]?.callEndReason?.code).toBe(1); |
Member
There was a problem hiding this comment.
Is this because the return on these callEndReasons are strings?
Member
Author
There was a problem hiding this comment.
The ? is added because context.getState().callsEnded[mockCallId] now correctly returns Call | undefined (importantly the | undefined is now added to typescripts interpretation of the return type because the call is accessed via object[key] where key isn't guaranteed to exist)
emlynmac
approved these changes
Oct 4, 2024
Member
emlynmac
left a comment
emlynmac
left a comment
There was a problem hiding this comment.
Nice to see this change going in!
Contributor
|
Storybook 8 URL https://60c7ae6891f0e90039d7cd54-zacflulyeu.chromatic.com/ |
This was referenced Oct 5, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
noUncheckedIndexedAccesstotrueWhy
We have had several bugs stemming from this issue (often these lead to app crashes).
As an example: What happens is
const a: string = some_array[5];will typeaasstring. However this is not always the case! ifsome_array[5]is beyond the range of the array thenaisundefined. This causes exceptions, for example ifa.someFn()is called, it will throw sayingsomeFndoes not exist onundefined. Really a should've been typed asstring | undefinedthen developer handles the undefined case. This is easily gated and handled at compile time withnoUncheckedIndexedAccessflag.How Tested
CI only