Introduce Membership TS type (take 2)

[andybalaam]

This is an updated version of #2723. Original description:

According to the Matrix Spec the room membership is enum with finite numbers of possible states.

This PR introduces a new typescript type Membership to be able to strictly specify what the exact membership is instead of > the generic string type. The scope includes:

• New Membership type
• Update type definitions in Client, Room, RoomMember and EventContent interfaces
• Update existing conditions to use the new type instead of hardcoded strings
• Update spec files with the new type

Signed-off-by: Stanislav Demydiuk s.demydiuk@gmail.com

After a discussion in our weekly meeting, we agreed this should be re-done using an enum and a union with string. This means we don't break any existing code, and we allow membership types that we are not aware of. The downside is that we can only enforce use of the enum using coding standards instead of the compiler.

Changelog: Introduce Membership TS type (#4107). Contributed by @stas-demydiuk

[richvdh]

could you give these things docstrings please.

[richvdh]

Any idea what @types/partials is meant to be about, and how it differs from (say) @types/event ?

Also: I slightly wonder whether this should be in the new types entrypoint rather than adding to the clutter of the main namespace, but then apparently that is meant to be types-only which makes it no god for enums. OTOH, I'm not convinced that defining the new entrypoint to be types-only was a good decision, for this reason.

WDYT?

[andybalaam]

Any idea what @types/partials is meant to be about, and how it differs from (say) @types/event ?

No. The original contributor put it here, so I left it.

[andybalaam]

I slightly wonder whether this should be in the new types entrypoint

I haven't been following this conversation closely, so I don't know and am happy to be guided. Is the types-only decision documented somewhere? Surely an enum is a type? Or does "type" here mean "stuff that disappears entirely at compile time"?

[t3chguy]

An enum is not a type-only export, it maps to an Object literal in JS.

Or does "type" here mean "stuff that disappears entirely at compile time"?

It means something you use export type / import type on

[andybalaam]

Docstrings added in d1cc8e9

[richvdh]

Is the types-only decision documented somewhere?

Well, it turns out my memory is faulty here. My memory of #4092 (which introduced types.ts) was that the README says words to the effect of "when you import from matrix-js-sdk/lib/types.ts, you should import type it". It turns out that is not the case: rather it uses the woollier wording "some low level TypeScript types". Which, IMHO, is fine, and can justifiably include enums.

Surely an enum is a type? Or does "type" here mean "stuff that disappears entirely at compile time"?

So yes, due to my faulty memory, I was talking about "stuff that disappears entirely at typescript-compile time", aka something that you can import type on. To be clear: an enum is not such a thing. (If you import type an enum, you can it as a type annotation, but can't use the values within in javascript expressions. Yay enums.)

Anyway: because the README uses the woolly wording, I think it's fine for KnownMembership and Membership to be exported via types.ts rather the main entrypoint; and, I think we should try to stop cluttering up the main entrypoint with more and more types.

In other words: my opinion is that these types should be moved into types.ts. (Or, maybe better, another file which types.ts re-exports but is not re-exported by index.ts.)

[andybalaam]

Done in 485f69c

[andybalaam]

@uhoreg @dbkr you were assigned this as code owners of different areas. Any chance you could take a quick look? Thanks!

[uhoreg]

Looks fairly sane

[richvdh]

@uhoreg @dbkr you were assigned this as code owners of different areas.

Just for clarity, as a matter of process: although @uhoreg was nominated as an owner of element-crypto-web-reviewers, a review from any member of that group (including me!) is sufficient. We do, however, need a review from a member of element-call-reviewers such as @dbkr ...

[robintown]

I can't say I understand the value of this change if it doesn't make the types any stricter, and creates a new code style requirement that we're supposed to enforce manually. The Rust SDK seems to have locked down its membership types like the original PR was proposing - is there anything we can learn from its approach? (like, does it not assume that it'll be compatible with arbitrary room versions from the future?)

[andybalaam]

I can't say I understand the value of this change if it doesn't make the types any stricter, and creates a new code style requirement that we're supposed to enforce manually. The Rust SDK seems to have locked down its membership types like the original PR was proposing - is there anything we can learn from its approach? (like, does it not assume that it'll be compatible with arbitrary room versions from the future?)

I agree that this change would have more value if it strictly enforced the types, but it would also make it impossible for new membership types to be used with matrix-js-sdk. We discussed this in the weekly team meeting and chose this implementation strategy. I do think it offers some value: it makes it easy to see which membership types are available, and it prevents typos in those strings. It should be easy to follow the convention because all the surrounding code uses it.

I think the question you raised would be an excellent thing to discuss in a future meeting or in #element-dev:matrix.org but I don't think we should block this PR based on it: it's a wider question of changing our policy.

[t3chguy]

You could always do something like KnownMembership | Exclude<string, "join" | "leave" | ...>

[andybalaam]

You could always do something like KnownMembership | Exclude<string, "join" | "leave" | ...>

Nice idea, but I think that would be an interface change, right? So I'd advocate for doing it separately from this change.

[richvdh]

looks great!