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

Fix addEventCallback input parameter type #6353

Merged
merged 4 commits into from Aug 16, 2023
Merged

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Aug 16, 2023

Fixes input parameter type for addEventCallback which broke when moving from v2 to v3

Addresses #6309

@github-actions github-actions bot added the msal-browser Related to msal-browser package label Aug 16, 2023
@tnorling tnorling linked an issue Aug 16, 2023 that may be closed by this pull request
@sameerag
Copy link
Member

Can we add tests to match the type?

Copy link
Contributor

@peterzenz peterzenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything we can do to properly test for this?

Copy link
Collaborator

@jo-arroyo jo-arroyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, Thomas!

@tnorling
Copy link
Collaborator Author

Generally speaking testing types is done by the TypeScript compiler, you can't really write unit tests against non-primitive types because types are stripped at runtime. Functionally nothing changes here, this is purely a TypeScript change.

We do have tests for the addEventCallback API, however, they continued to pass because PCA was expecting a less specific type. Now that we've restored the expectation for a more specific type attempts to pass in a less specific type would fail at the TypeScript level.

@tnorling tnorling merged commit 212d1ba into dev Aug 16, 2023
28 of 29 checks passed
@tnorling tnorling deleted the fix-addEventCallback-type branch August 16, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PublicClientApplication.addEventCallback has incorrect callback type
5 participants