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

Bump @matrix-org/react-sdk-module-api from 1.0.0 to 2.0.0 #25986

Merged

Conversation

dhenneke
Copy link
Contributor

@dhenneke dhenneke commented Aug 17, 2023

Depends on matrix-org/matrix-react-sdk#11395.

This PR includes two more changes:

1. Have Element compatible with multiple versions of the Module API at the same time

Based on the discussion in #element-dev:matrix.org. Gist: The semantic version of the Module API is not a good indicator for the compatibility of a module to the client. Element can accept modules that run on older versions of the module api as long as the Module-Surface-API stays compatible. If any non-backwards compatible change to the Module part of the API happens, this list must be reset.

2. Make sure only a single copy of the Module API is used

This was already the case without 1., but now it is inevitable. Some parts of the Module API rely on global variables, so every module must use the same instance of the module API package. 1. requires that all modules can work with all compatible versions of the Module API, so it should be acceptable to let webpack force to resolve it to a single instance.

An example where this breaks. Note that left shows the fallback textfield and right the correct component.

develop branch this PR
image image

(checkout develop, add "@vector-im/element-web-ilag-module@0.0.4" to the build_config.yaml, open the url to a public room in an incognito tab, and try to join as guest)

This error will only be testable when we have e2e tests for the Module API. I hope this discussion doesn't block this PR.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

Signed-off-by: Dominik Henneke <dominik.henneke@nordeck.net>
@dhenneke dhenneke requested review from a team as code owners August 17, 2023 11:17
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Aug 17, 2023
// in the Client-side surface lead to a major bump even though the Module-side surface stays
// compatible. We aim to not break the Module-side surface so we maintain a list of compatible
// older versions.
const backwardsCompatibleMajorVersions = ["1.0.0"];
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine to me for now but I wonder if this wouldn't easily get unwieldy in future? Imagine we end up having a couple more backwards compatible versions in this list. Wouldn't the act of deciding whether or not a new version is backwards-compatible with all of the versions listed here become ever more cumbersome?

If this is a problem with differentiating between the client-facing and the module-facing API version, could we somehow define separate version numbers for these two?

CC @t3chguy / @richvdh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think one issue here is that @matrix-org/react-sdk-module-api (and also matrix-widget-api) are both no APIs that should be updated in an automated fashion because they require manual changes in the matrix-react-sdk most of the time.

Copy link
Member

Choose a reason for hiding this comment

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

@Johennes: we discussed this in #element-dev yesterday. I suggested this approach as a pragmatic solution to the compatibility problem for now. Changing the way we manage API versions altogether would involve a bunch more thinking and work, and we can do it in future if and when this approach becomes unmanageable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I've followed that discussion and agreed that this seems fine for the moment. Point taken on us returning to this once it does actually become unwieldy.

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

@t3chguy t3chguy enabled auto-merge (squash) August 17, 2023 17:03
@richvdh richvdh added the T-Task Tasks for the team like planning label Aug 18, 2023
@t3chguy t3chguy merged commit 65f7545 into element-hq:develop Aug 18, 2023
26 of 27 checks passed
@dhenneke dhenneke deleted the nic/feat/module-api-dialog-improvements branch August 21, 2023 11:41
@Johennes Johennes added A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project and removed Z-Community-PR Issue is solved by a community member's PR labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants