feat: Bundle pwd confirmation as an interceptor #714
Conversation
We are migrating to in-request password confirmation, so it makes sens to have all that logic in this repository. Most of the code is a copy-past from the `@nc/password-confirmation` repository. Then there is the addition of a new interceptor to use the pwd confirmation logic. Signed-off-by: Louis Chemineau <[email protected]>
artonge
force-pushed
the
artonge/bundle_pwd_confirmation
branch
from
f170450 to
cdb1c48
Compare
Signed-off-by: Louis Chemineau <[email protected]>
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
Really nice work!
One thing about the interceptors: I do not really see the use case of the id you use on the requests (and the maps).
Because you can simply await either the confirmation or the dialog response.
And for the case of multiple requests with authentication (could happen if you put the request in async functions), we should simply delay those requests until the current confirmation is done.
This is especially for UX, as otherwise multiple dialogs could stack over each other.
With the current approach multiple different password confirmations could stack, even if the first would unblock.
E.g.:
- request A with lax confirmation
- A dialog is shown, request to confirm is sent
- request B with lax confirmation
- B dialog is shown
- A resolves successfully
- B sends the request again
Instead B should be waited until A is fully done, then in this case no confirmation is even needed for B.
| // theFileYouDeclaredTheCustomConfigIn.ts | ||
| declare module 'axios' { | ||
| export interface AxiosRequestConfig { | ||
| confirmPassword?: 'reminder'|'inRequest'; |
There was a problem hiding this comment.
I find the name a bit confusing, how about strict and lax?
Just personal opinion, so feel free to ignore and mark as resolve.
| import { onError as onCsrfTokenError } from './interceptors/csrf-token' | ||
| import { onError as onMaintenanceModeError } from './interceptors/maintenance-mode' | ||
| import { onError as onNotLoggedInError } from './interceptors/not-logged-in' | ||
| import { addPasswordConfirmationInterceptors } from './password-confirmation' |
There was a problem hiding this comment.
Maybe we should also export this function? Might be useful if a custom Axios instance is created.
|
Closed in favor of nextcloud-libraries/nextcloud-password-confirmation#881 |
We are migrating to in-request password confirmation, so it makes sens to have all that logic in this repository.
Most of the code is a copy-past from the
@nc/password-confirmationrepository.Then there is the addition of a new interceptor to use the pwd confirmation logic.