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

GIS-866: Search Communication #647

Merged
merged 17 commits into from Jan 7, 2021

handle previously unhandled fetch rejections

  • Loading branch information
fcjr committed Jan 5, 2021
commit 0ca6af23a5eb6da4a85a56b41e4b7332395fdd9e
@@ -29,15 +29,21 @@ class Api {

refreshToken() {
This conversation was marked as resolved by jsignanini

This comment has been minimized.

@jsignanini

jsignanini Jan 4, 2021
Member

@fcjr I'm thinking renaming this to something like refreshTokenAndHandleIsRefreshing to make it super clear what the purpose of this new function is

This comment has been minimized.

@fcjr

fcjr Jan 4, 2021
Author Member

That sounds reasonable, I'll make the change. My thought with just naming it refreshToken was that as an external user of the api class the inner "isRefreshing" state is an implementation detail that external users of the function don't really need to know about, but since this should only be used in special cases anyways there shouldn't be a harm in making it explicit

This comment has been minimized.

@jsignanini

jsignanini Jan 4, 2021
Member

@fcjr actually you do make a good point as it’s only used internally in the other “private” function. Ok you can just leave it as is then. Thanks.

if (this.isRefreshing) {
let bindedResolve;
const _processRefreshTokenEvent = (resolve, e) => {
window.removeEventListener(this.tokenRefreshedEventType, bindedResolve, false);
resolve(e.detail);
};
return new Promise((resolve) => {
bindedResolve = _processRefreshTokenEvent.bind(null, resolve);
window.addEventListener(this.tokenRefreshedEventType, bindedResolve, false);
const deferedPromise = {};
deferedPromise.p = new Promise((resolve, reject) => {
deferedPromise.resolve = resolve;
deferedPromise.reject = reject;
});
const _processRefreshTokenEvent = (e) => {
window.removeEventListener(this.tokenRefreshedEventType, deferedPromise, false);
if (e.detail && e.detail.err) {
deferedPromise.reject(e.detail.err);
} else {
deferedPromise.resolve(e.detail);
}
};
window.addEventListener(this.tokenRefreshedEventType, _processRefreshTokenEvent, false);
return deferedPromise.p;
}

this.isRefreshing = true;
@@ -50,6 +56,12 @@ class Api {
detail: res,
}));
return res;
}).catch((err) => {
this.isRefreshing = false;
window.dispatchEvent(new CustomEvent(this.tokenRefreshedEventType, {
detail: { err },
}));
throw err;
This conversation was marked as resolved by fcjr

This comment has been minimized.

@jsignanini

jsignanini Jan 5, 2021
Member

@fcjr could you move the two this.isRefreshing = false to just one in a .then() after the .catch()?

});
}

ProTip! Use n and p to navigate between commits in a pull request.