Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Conversation

@thoraj
Copy link
Contributor

@thoraj thoraj commented Dec 21, 2023

Background

The purpose of this PR is to provide a replacement for the deprecated Security customisations. See #24

The PR brings two main contributions:

  • A mechanism to enable defining a set of functions which can be called from appropriate call sites in react-sdk
  • Definition and implemention (CryptoSetupExtensions) which will replace the now deprecated Security customisations.

Method call extension mechanism

This will be in addition to the already existing mechanism to raise lifecycle events, so (if this PR is accepted), there will be two main ways to extension modules to "interact" with react-sdk:

  • Raising events using
moduleRunner.invoke(...)
  • Calling methods exposed on the moduleRunner instance using e.g.
const key = moduleRunner.extensions.cryptoSetup.getSecureBackupKey(...)

The reason both are needed is that the semantics of raising events, and calling methods is fundamenally different. Raising events is fire and forget, and the calling code is indifferent to whether anything is listening or not. Therefore raising events is not an appropriate mechanism for returning data, and possibly changing program flow.

The method call semantics are appropriate if e.g. program flow should be modified based on the results of the call (which is how the deprecated Security customisation works).

Introducing an extension

Extensions are introduced by adding code in this repo and in matrix-react-sdk. This of course means that new extensions are introduced by the Element team either by merging constributions (like this one), or by authoring new extensions themselves. regardless, to create a new method call one must:

  • Define an interface describing the extension methods.
  • Providing a default implementation. This will define the behavior if no extension modules provide an implementation for the extension.
  • Changing the react-sdk by:
    • Adding call sites
    • Possibly taking actions based on the results of the calls

Implementing an extension in an extension module

Extension modules are created by developers wishing to extend EW, by deriving from the RuntimeModule base class. This PR adds a property to expose extension methods. An extension module may or may not provide an implementation of the available extension method interface. However, only one extension module can provide an implementation of an extension method interface. This is enforced in react-sdk when the extension modules are registered (and thus not part of this PR)

Changes to the matrix-react-sdk (separate PR)

For everything to come together, there will changes needed in matrix-react-sdk as well. These will be proposed in a separate PR which will show the details, but in short the PR will:

  • Change the call sites for the Security customisations, and replace the calls with methods provided by the new extension point CryptoSetupExtensions
  • Change ModuleRunner to ensure extensions defined for a RuntimeModule is exposed on the ModuleRunner instance

Future extensions

Although the scope of this PR is to address #24, it would also provide an easy way to port other customisations such as e.g. ComponentVisibility.ts

Signed-off-by: Thor A. Johansen taj@verji.com

@thoraj thoraj marked this pull request as ready for review December 21, 2023 10:41
@thoraj thoraj requested a review from a team as a code owner December 21, 2023 10:41
@thoraj thoraj requested review from dbkr and t3chguy December 21, 2023 10:41
@t3chguy
Copy link
Member

t3chguy commented Jan 10, 2024

Don't forget to request a re-review when it is ready for such.

image

@thoraj
Copy link
Contributor Author

thoraj commented Jan 10, 2024

Not sure how to request another review. On my side it looks like this:
image

(review from @dbkr already requested, and no button to re-request review from @t3chguy)

@thoraj thoraj requested a review from t3chguy January 10, 2024 20:57
@thoraj
Copy link
Contributor Author

thoraj commented Jan 17, 2024

I hope I requested a re-review correctly?

I understand you are very busy, but it would be great to know what to expect wrt lead time, and when this can/will happen?
If the PR is approved (and released), it will be much simpler to create accompanying PR for matrix-react-sdk, and also to prepare porting other customisations.

@thoraj
Copy link
Contributor Author

thoraj commented Jan 22, 2024

@t3chguy ping ☝️

@thoraj
Copy link
Contributor Author

thoraj commented Jan 29, 2024

We are eager to continue our work and make contributions to the EW ecosystem.

If what is missing in the PR is cleaning up formatting and code style, we can go ahead and make additional PRs for finishing the extension, as well as porting more customisations. If you have issues with the design and implementation in this PR, we will definitely wait for your re-review before spending more time.

Either way it would be great if we could nudge this PR along, so we kindly ask if we can get a re-review (@t3chguy @dbkr )

@t3chguy
Copy link
Member

t3chguy commented Jan 29, 2024

Sorry I was away sick @thoraj. I will try get to it today but have a significant amount to catch up on.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants