IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 841 forks source link

Bug: UserManager.signoutCallback's type specifies a return type of 'SignoutResponse | void' instead of 'SignoutResponse | undefined' (PR Available) #1193

Closed Daniel-Khodabakhsh closed 3 years ago

Daniel-Khodabakhsh commented 4 years ago

The typing for UserManager.signoutCallback currently makes this method return SignoutResponse | void.

Although legal, variables should not be of type void, from the typescript specifications:

NOTE: We might consider disallowing declaring variables of type Void as they serve no useful purpose. However, because Void is permitted as a type argument to a generic type or function it is not feasible to disallow Void properties or parameters.

Additionally, attempting to use a variable of type SignoutResponse | void won't let us treat it as null or undefined like in the next example:

const response: SignoutResponse | void = await userManager.signoutCallback(window.location.href);
doSomething(response?.state?.someStateItem);
// This lints 'state' with : TS2339 (TS) Property 'state' does not exist on type 'void | SignoutResponse'.

Using undefined or null for UserManager.signoutCallback works for the above example and is semantically correct because there is a chance SignoutResponse or nothing is returned (undefined).

Using void here is not semantically correct because UserManager.signoutCallback can return something, even if that something is undefined. Also, using void forces developers to hack around userManager.signoutCallback's return type in order to use strict typing.

Daniel-Khodabakhsh commented 4 years ago

A PR to fix this issue is available: https://github.com/IdentityModel/oidc-client-js/pull/1189

Daniel-Khodabakhsh commented 4 years ago

Hey @brockallen would you be able to take a look at my PR? Or do you now who I can ping? https://github.com/IdentityModel/oidc-client-js/pull/1189

Daniel-Khodabakhsh commented 3 years ago

Pull request #1189 merged.