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

Support Futures via widgets #4311

Merged
merged 1 commit into from
Aug 1, 2024
Merged

Support Futures via widgets #4311

merged 1 commit into from
Aug 1, 2024

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Jul 11, 2024

Signed-off-by: Andrew Ferrazzutti andrewf@element.io

Dependencies:

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

src/embedded.ts Outdated Show resolved Hide resolved
@AndrewFerr
Copy link
Member Author

Putting this back into draft until the widget API is up-to-speed with the recent revisions to MSC4140.

Base automatically changed from af/msc4140 to develop July 30, 2024 13:01
@AndrewFerr
Copy link
Member Author

I'll take this out of draft once the dependent widget-api PR is merged.

@AndrewFerr
Copy link
Member Author

I'll take this out of draft once the dependent widget-api PR is merged.

Not quite -- it would be nice if the widget-api's release workflow could be fixed before cutting a new release for it, which this PR will depend on.

@AndrewFerr
Copy link
Member Author

After the rebasing, the effective changes since the last review start from 3c65057.

@AndrewFerr AndrewFerr self-assigned this Aug 1, 2024
Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

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

Looks very good

src/embedded.ts Show resolved Hide resolved
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

One non-blocking comment:

Comment on lines +343 to +345
if (!(await this.doesServerSupportUnstableFeature(UNSTABLE_MSC4140_DELAYED_EVENTS))) {
throw Error("Server does not support the delayed events API");
}
Copy link
Member

Choose a reason for hiding this comment

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

It feels a little surprising that the host client might approve the capability without having server support. (I would expect this check to be the host's responsibility, ideally)

@AndrewFerr AndrewFerr requested a review from t3chguy August 1, 2024 13:48
@@ -268,7 +268,8 @@ describe("MSC4108SignInWithQR", () => {
it("should abort if device doesn't come up by timeout", async () => {
jest.spyOn(global, "setTimeout").mockImplementation((fn) => {
(<Function>fn)();
return -1;
// TODO: mock timers properly
return -1 as any;
Copy link
Member

Choose a reason for hiding this comment

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

Why are these needed now but weren't before?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually sure...it must be due to a dependency update. Typechecks expect the return value to be a NodeJS Timeout instead of a timeout ID.

@AndrewFerr AndrewFerr added this pull request to the merge queue Aug 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 1, 2024
@t3chguy t3chguy added this pull request to the merge queue Aug 1, 2024
Merged via the queue into develop with commit e10c362 Aug 1, 2024
26 checks passed
@t3chguy t3chguy deleted the af/msc4157 branch August 1, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants