-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Remove shouldRetry and canCancel now that deprecatedAPI is gone #19675
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
Changes from all commits
f827a2b
c80e6cd
a3be1c2
e6caa84
50dead1
bc6bfe0
a0322a9
564c9e1
8e647fa
f8e16e0
51eb0e7
d2f0e0b
7121430
3245c23
a498ab0
9a63feb
5558365
63ad264
ca3fd6b
0d2c062
f394462
c872b58
0940980
1e64251
e83f204
e6089d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -58,30 +58,18 @@ function Reauthentication(response, request, isFromSequentialQueue) { | |||||
// There are some API requests that should not be retried when there is an auth failure like | ||||||
// creating and deleting logins. In those cases, they should handle the original response instead | ||||||
// of the new response created by handleExpiredAuthToken. | ||||||
const shouldRetry = lodashGet(request, 'data.shouldRetry'); | ||||||
// If the request was from the sequential queue we don't want to return, we want to run the reauthentication callback and retry the request | ||||||
madmax330 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
const apiRequestType = lodashGet(request, 'data.apiRequestType'); | ||||||
|
||||||
// For the SignInWithShortLivedAuthToken command, if the short token expires, the server returns a 407 error, | ||||||
// and credentials are still empty at this time, which causes reauthenticate to throw an error (requireParameters), | ||||||
// and the subsequent SaveResponseInOnyx also cannot be executed, so we need this parameter to skip the reauthentication logic. | ||||||
const isDeprecatedRequest = !apiRequestType; | ||||||
const skipReauthentication = lodashGet(request, 'data.skipReauthentication'); | ||||||
if ((!shouldRetry && !apiRequestType) || skipReauthentication) { | ||||||
if (isFromSequentialQueue) { | ||||||
return data; | ||||||
} | ||||||
madmax330 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
if (!isFromSequentialQueue && (isDeprecatedRequest || skipReauthentication)) { | ||||||
if (request.resolve) { | ||||||
request.resolve(data); | ||||||
return; | ||||||
} | ||||||
return data; | ||||||
} | ||||||
|
||||||
// We are already authenticating and using the DeprecatedAPI so we will replay the request | ||||||
if (!apiRequestType && NetworkStore.isAuthenticating()) { | ||||||
MainQueue.replay(request); | ||||||
return data; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB / more clean up (that maybe we should just do now) Is it accurate to say at this point that anything below this must be from the new API? I think so right? Because we return early if So all the stuff with the Can we remove this?
And this:
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we can remove these, let's investigate in a follow up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Maybe we can add some logs for it in another PR. I can do it. I'd like to get rid of any dead code if can. |
||||||
|
||||||
return reauthenticate(request.commandName) | ||||||
.then((authenticateResponse) => { | ||||||
if (isFromSequentialQueue || apiRequestType === CONST.API_REQUEST_TYPE.MAKE_REQUEST_WITH_SIDE_EFFECTS) { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -340,7 +340,7 @@ function beginDeepLinkRedirect() { | |||||
} | ||||||
|
||||||
// eslint-disable-next-line rulesdir/no-api-side-effects-method | ||||||
API.makeRequestWithSideEffects('OpenOldDotLink', {shouldRetry: false}, {}).then((response) => { | ||||||
marcaaron marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
API.makeRequestWithSideEffects('OpenOldDotLink', {}, {}).then((response) => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Not a blocker |
||||||
Browser.openRouteInDesktopApp(response.shortLivedAuthToken, currentUserEmail); | ||||||
}); | ||||||
} | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.