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

Initial surface for module API (enough for an ILAG module) #1

Merged
merged 7 commits into from
May 25, 2022

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Apr 6, 2022

This layer is intended to represent the actual module API surface for element-web (technically react-sdk). We extract this to a dedicated package for a few reasons:

  1. It makes determining module compatibility trivial: compare the module-api dependency of the element-web layer and the module with semver to see if it'll work.
  2. This package is mostly just types and very little code, making for a small dependency. A module would not need to pull in the entirety of the react-sdk or similar just to make itself compatible.
  3. With a dedicated package, modules are less likely to reach into implementation-specific code to "go around" the module API surface. For example: modules will have a harder time getting access to a MatrixClientPeg (not impossible, but harder).

With this PR, the project gets shifted over to the element-web team for ongoing maintenance. It is treated as a direct dependency of the react-sdk itself and thus maintained by the same team.

Note: There are other layers which are listed as drafts. These drafts are not up for review because they depend on layers like this one to exist and be published. They will be put up for review incrementally.

Upon merge of this PR, I'll publish the package to npm.


Part of a series / reference material:

@turt2live turt2live marked this pull request as ready for review May 12, 2022 23:17
@turt2live turt2live requested a review from a team as a code owner May 12, 2022 23:17
@t3chguy
Copy link
Member

t3chguy commented May 20, 2022

With this PR, the project gets shifted over to the element-web team for ongoing maintenance. It is treated as a direct dependency of the react-sdk itself and thus maintained by the same team.

There are other prerequisites for this

Such as copying the T- labels, wiring up a bunch of the same CI (Enforce Labels, Sonar, Static Analyses, etc)

src/types/credentials.ts Outdated Show resolved Hide resolved
src/ModuleApi.ts Outdated Show resolved Hide resolved
src/ModuleApi.ts Outdated Show resolved Hide resolved
src/ModuleApi.ts Outdated Show resolved Hide resolved
src/ModuleApi.ts Outdated Show resolved Hide resolved
src/ModuleApi.ts Outdated Show resolved Hide resolved
Comment on lines +21 to +22
// TODO: Type the event emitter with AnyLifecycle (extract TypedEventEmitter from js-sdk somehow?)
// See https://github.com/matrix-org/matrix-react-sdk-module-api/issues/4
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this just grow a peer-dep to js-sdk given it knows it'll have a js-sdk from react-sdk?

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially. I've been trying to avoid the peer dependency style as it'll end up being a slew of warnings for developers (because we won't update the dependency very often)

Copy link
Member

Choose a reason for hiding this comment

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

that's fine though because its a peer-dep, you could even specify the min version and say any version from there. >=12.4.1 (replacing with the actual version we introduced TypedEventEmitter)

Copy link
Member Author

Choose a reason for hiding this comment

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

until we release a breaking change of the js-sdk and then it is considered unmatched (both logically and by the semver checks - we can't assume it'll continue to compile).

For now I'm not that concerned with typed event emitter support. Documentation can supplement it.

Copy link
Member

Choose a reason for hiding this comment

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

Then why not add yet another layer called matrix-js-common or something :P?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if serious, but it's quite literally on the table for consideration.

Copy link
Member

Choose a reason for hiding this comment

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

It'd be a good home for some things for element-desktop to re-use too, like fetchdep.sh

Copy link
Member Author

Choose a reason for hiding this comment

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

probably for a larger conversation outside of this PR though ;)

* @param props The component properties
* @returns The component, rendered.
*/
public static renderFactory = (props: TextInputFieldProps): React.ReactNode => (
Copy link
Member

Choose a reason for hiding this comment

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

This pattern will be very easy to forget about when refactoring, it is very strange

could we not move the module-api up to element-web and then module-api can depend on react-sdk and use components like Spinner etc inherently?

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're actively avoiding a react SDK dependency because the modules themselves cannot and should not have easy access to that dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Bit more reasoning here would be great

Copy link
Member Author

@turt2live turt2live May 24, 2022

Choose a reason for hiding this comment

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

The layer provides two main benefits:

  1. It gives us something to check version against implicitly, ensuring we're only including supported modules in our build.
  2. It avoids modules depending on the react-sdk either directly or indirectly, reducing risk of developers breaking free of the module system and making bad decisions. The react-sdk is also not a tiny project, so it ultimately saves disk space on developer machines, and it reduces the chances of dependency conflicts when webpack tries to figure out what is going on.
    • A react-sdk dependency would also overly tie a module to an application version when in fact it could likely be supported across breaking changes of the react-sdk: this layer abstraction allows for a module to write code against a standard set of functions and be fine for several releases. Essentially, it allows us to break the app and module API surface separately, a benefit that projects like Synapse don't have.

Together, these are reason enough to warrant the new layer. While the layer itself is a bit annoying (requires someone to keep an eye on it), it's expected to be even less traffic than the widget-api or analytics-events dependencies.

@turt2live
Copy link
Member Author

With this PR, the project gets shifted over to the element-web team for ongoing maintenance. It is treated as a direct dependency of the react-sdk itself and thus maintained by the same team.

There are other prerequisites for this

Such as copying the T- labels, wiring up a bunch of the same CI (Enforce Labels, Sonar, Static Analyses, etc)

this is all counted as "ongoing maintenance" btw :P (we don't have any of this infrastructure on the widget-api, for instance)

also, considering it's a bit in flux at the moment I'd rather switch once we have everything set up fully on the other layers. There's also no real reason to overload this PR at the moment (I think).

@turt2live turt2live requested a review from t3chguy May 25, 2022 03:21
@turt2live turt2live merged commit 7a78f98 into main May 25, 2022
@turt2live turt2live deleted the travis/modules branch May 25, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants