[Identity] Getting rid of instanceof #14763
Conversation
I had some isntances of `instanceof`, which is causing issues once @azure/msal-common gets updated, since @azure/msal-node hasn't been released with the latest version of @azure/msal-common. I know that checking for `names` is the way to go, but I had forgotten to remove these before. This will fix `rush update --full`.
ghost
added
the
sdk/identity/identity/CHANGELOG.md
Outdated
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| ## 2.0.0-beta.2 (Unreleased) | |||
|
|
|||
| - Bug fix: Replaced cases where `instanceof` was used to compare errors with comparisons on the error names. Uneven updates on our dependencies would cause `instanceof` to mismatch our error comparisons. Comparing by error name makes sure dependency changes won't alter our logic. | |||
There was a problem hiding this comment.
Can we specify when this issue got introduced? For example if this is something that was introduced in v2 beta 1, then v1 users in the future will not have to care about it
There was a problem hiding this comment.
v1 had these instances of instanceof:
src/credentials/managedIdentityCredential/index.ts:179: if (err instanceof CredentialUnavailable) {
src/credentials/managedIdentityCredential/imdsMsi.ts:86: (err instanceof RestError && err.code === RestError.REQUEST_SEND_ERROR) ||
src/credentials/interactiveBrowserCredential.ts:100: if (e instanceof AuthenticationRequired) {
src/credentials/interactiveBrowserCredential.browser.ts:97: if (err instanceof msal.AuthError) {
src/credentials/deviceCodeCredential.ts:131: if (e instanceof AuthenticationRequired) {
src/credentials/chainedTokenCredential.ts:67: if (err instanceof CredentialUnavailable) {
Instanceof was an issue only on interactiveBrowserCredential.browser.ts, which uses the older MSAL that is deprecated. For v1, I don't think this is an issue we should worry about at this point.
Should I remove the changelog entry then?
There was a problem hiding this comment.
In that case, we can skip the changelog entry. Not much use to the end user
| @@ -149,24 +149,25 @@ export class MsalBaseUtilities { | |||
| * Handles MSAL errors. | |||
| */ | |||
| protected handleError(scopes: string[], error: Error, getTokenOptions?: GetTokenOptions): Error { | |||
| if (error instanceof msalCommon.AuthError) { | |||
| switch (error.errorCode) { | |||
| if (error.name === "AuthError" || error.name === "ClientAuthError") { | |||
There was a problem hiding this comment.
Is ClientAuthError a separate error class? If so, then am guessing that the previous instanceOf check did not work for this case?
There was a problem hiding this comment.
ClientAuthError inherits AuthError. The previous one worked for both.
There was a problem hiding this comment.
Do we have confirmation from the msal team that there are no other error classes that inherit AuthError?
There was a problem hiding this comment.
These are all the instances of things that extend AuthError:
./lib/msal-browser/src/error/BrowserAuthError.ts:149:export class BrowserAuthError extends AuthError {
./lib/msal-browser/src/error/BrowserConfigurationAuthError.ts:47:export class BrowserConfigurationAuthError extends AuthError {
./lib/msal-common/src/error/ServerError.ts:11:export class ServerError extends AuthError {
./lib/msal-common/src/error/ClientAuthError.ts:179:export class ClientAuthError extends AuthError {
./lib/msal-core/src/error/ServerError.ts:21:export class ServerError extends AuthError {
./lib/msal-core/src/error/ClientAuthError.ts:96:export class ClientAuthError extends AuthError {
I can see some improvements I can do. I'll push another commit.
There was a problem hiding this comment.
I have pushed this commit that considers BrowserAuthError and BrowserConfigurationAuthError! Good catch! Thank you! d67b645
There was a problem hiding this comment.
One concern here is that if msal-common adds new error classes that keep extending from AuthError, we will have to keep playing catch up. Just something to keep in mind I guess.
sdk/identity/identity/CHANGELOG.md
Outdated
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| ## 2.0.0-beta.2 (Unreleased) | |||
|
|
|||
| - Bug fix: Replaced cases where `instanceof` was used to compare errors with comparisons on the error names. Uneven updates on our dependencies would cause `instanceof` to mismatch our error comparisons. Comparing by error name makes sure dependency changes won't alter our logic. | |||
There was a problem hiding this comment.
Can you add a specific example?
Like..
Previously, if the msal@1.4.9 version is installed, "ClientAuthError" is thrown instead of "AuthError" in some cases whereas, for msal@1.4.8, you'd see the same "AuthError". And this PR #14763 attempts to make the errors returned consistent across versions.
Or something in those lines?
There was a problem hiding this comment.
Based on this: #14763 (comment) I'll remove the changelog entry.
Added FPGA and GPU Node type family for all stable versions (Azure#14763) * Added FPGA and GPU Node type family for all stable versions Added FPGA and GPU Node type family for all stable versions * adding node size family for 2021-04-01-preview Co-authored-by: Sai-Kumar-1901 <saikumar@microsoft.com>
I had some instances of
instanceof, which is causing issues once @azure/msal-common gets updated, since @azure/msal-node hasn't been released with the latest version of @azure/msal-common.For background: instanceof will not match for two different class definitions with the same name loaded on runtime. They can look similar, but if they're not the same exact class definition, instanceof won't match. This dependency mismatch causes instanceof to fail, since both versions of a same dependency are being loaded, causing the runtime to have two classes with the same name.
I know that checking for
error.nameis the way to go, but I had forgotten to remove these before.After this, there are no further instances of
instanceofin Identity's code.This will fix the
rush update --fullPRs.