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

Improve types around User Interactive Auth #4709

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Feb 13, 2025

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible. Could you update the PR title to be something that will make more sense in a changelog?

/**
* Helper type to represent HTTP response body for a UIA enabled endpoint
*/
export type UIAResponse<T> = T | IAuthData;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if we should (temporarily) make this

/** @deprecated */
export type UIAResponse<T> = T;

... to avoid having to synchronise the EW and js-sdk PRs.

No strong feelings though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need to synchronise them, EW can land first, shouldn't need to both land at once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, you're right, that is a better approach and will avoid a breaking change

@t3chguy t3chguy changed the title Remove confused type UIAResponse Improve types around User Interactive Auth Feb 13, 2025
@t3chguy t3chguy requested a review from richvdh February 13, 2025 13:40
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@richvdh
Copy link
Member

richvdh commented Feb 13, 2025

Requires #4709

wrong link, I think

Comment on lines -162 to +161
export type UIAuthCallback<T> = (makeRequest: (authData: AuthDict | null) => Promise<UIAResponse<T>>) => Promise<T>;
export type UIAuthCallback<T> = (makeRequest: (authData: AuthDict | null) => Promise<T>) => Promise<T>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice, while you're here (and since it took me a while to figure out), to clarify the documentation to say that, in the case of more auth being needed, makeRequest will throw a MatrixError whose data conforms to IAuthData. But I don't mind if you want to leave that for another time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect T-Deprecation A pull request that makes something deprecated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading Type: UIAResponse<T>, IAuthData is not valid response
3 participants