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

Combine QrCodeEvent, SasEvent and VerificationEvent #3386

Merged
merged 4 commits into from
May 22, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented May 19, 2023

I need to pull out an interface which is implemented by the VerificationBase implementations, but the magic around the event types that are emitted is getting in my way.

Everything is much easier if we declare a single enum that contains all the possible emitted events, rather than having separate enums for each implementation and the base class.

So, here we combine the three enums into one, and, for backwards compatibility, make the old enums be aliases to the new one (which should be safe as they are now supersets of the old ones).

Review commit-by-commit.

Notes: Deprecate QrCodeEvent, SasEvent and VerificationEvent: they are now replaced by a combined VerifierEvent.


Here's what your changelog entry will look like:

🚨 BREAKING CHANGES

  • Deprecate QrCodeEvent, SasEvent and VerificationEvent (#3386).

... and add some ✨comments✨
... as a precursor to extracting a single `Verifier` interface for `SAS` and `ReciprocateQRCode`.

`enum`s are slightly magical things that have both a type and a value, so we
have to re-export their backwards-compatibility fudges twice.
@richvdh richvdh requested a review from a team as a code owner May 19, 2023 10:32
@richvdh richvdh added the T-Deprecation A pull request that makes something deprecated label May 19, 2023
Comment on lines +55 to +58
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Events extends string = VerifierEvent,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Arguments = VerifierEventHandlerMap,
Copy link
Member Author

Choose a reason for hiding this comment

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

these type parameters are no longer used, but we need something here to maintain backwards compatibility with applications that reference VerificationBase.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add that as a comment?

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.

Seems fine to me

Comment on lines +55 to +58
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Events extends string = VerifierEvent,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Arguments = VerifierEventHandlerMap,
Copy link
Member

Choose a reason for hiding this comment

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

Could you add that as a comment?

@robintown robintown removed the request for review from weeman1337 May 19, 2023 19:58
@richvdh richvdh added this pull request to the merge queue May 22, 2023
Merged via the queue into develop with commit 614f446 May 22, 2023
@richvdh richvdh deleted the rav/element-r/11_combine_verifier_events branch May 22, 2023 11:14
@t3chguy
Copy link
Member

t3chguy commented May 23, 2023

matrix-org/matrix-react-sdk#10963 shows this is a breaking change

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Dec 13, 2023
* Ensure we do not add relations to the wrong timeline ([\matrix-org#3427](matrix-org#3427)). Fixes element-hq/element-web#25450 and element-hq/element-web#25494.
* Deprecate `QrCodeEvent`, `SasEvent` and `VerificationEvent` ([\matrix-org#3386](matrix-org#3386)).
* Move crypto classes into a separate namespace ([\matrix-org#3385](matrix-org#3385)).
* Mention deno support in the README ([\matrix-org#3417](matrix-org#3417)). Contributed by @sigmaSd.
* Mark room version 10 as safe ([\matrix-org#3425](matrix-org#3425)).
* Prioritise entirely supported flows for UIA ([\matrix-org#3402](matrix-org#3402)).
* Add methods to terminate idb worker ([\matrix-org#3362](matrix-org#3362)).
* Total summary count ([\matrix-org#3351](matrix-org#3351)). Contributed by @toger5.
* Audio concealment ([\matrix-org#3349](matrix-org#3349)). Contributed by @toger5.
* Correctly accumulate sync summaries. ([\matrix-org#3366](matrix-org#3366)). Fixes element-hq/element-web#23345.
* Keep measuring a call feed's volume after a stream replacement ([\matrix-org#3361](matrix-org#3361)). Fixes element-hq/element-call#1051.
* Element-R: Avoid uploading a new fallback key at every `/sync` ([\matrix-org#3338](matrix-org#3338)). Fixes element-hq/element-web#25215.
* Accumulate receipts for the main thread and unthreaded separately ([\matrix-org#3339](matrix-org#3339)). Fixes element-hq/element-web#24629.
* Remove spec non-compliant extended glob format ([\matrix-org#3423](matrix-org#3423)). Fixes element-hq/element-web#25474.
* Fix bug where original event was inserted into timeline instead of the edit event ([\matrix-org#3398](matrix-org#3398)). Contributed by @andybalaam.
* Only add a local receipt if it's after an existing receipt ([\matrix-org#3399](matrix-org#3399)). Contributed by @andybalaam.
* Attempt a potential workaround for stuck notifs ([\matrix-org#3384](matrix-org#3384)). Fixes element-hq/element-web#25406. Contributed by @andybalaam.
* Fix verification bug with `pendingEventOrdering: "chronological"` ([\matrix-org#3382](matrix-org#3382)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Deprecation A pull request that makes something deprecated X-Breaking-Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants