Skip to content

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Jul 11, 2024

Signed-off-by: Andrew Ferrazzutti [email protected]

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).

@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

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
(<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
@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