Use mapped types for account data content#4590
Conversation
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
|
FYI, I hate the fact that we refer to account data as "events". It's not an event by any reasonable definition of the word "event". (You could argue that a change in account data might count as an event when it is sent down I appreciate it might be scope-creeping this PR, and diverging from the current wording of the spec, in ways you don't want to take on, but if there was a way to create this PR without propagating the word "Event" further than we have to, it would make me much happier. |
|
I think for js-sdk naming consistency, given they use MatrixEvent under the hood, it should be tackled more holistically. Maybe even starting at the spec level. The spec does indeed seem to refer to Account Data events as message events in some places https://spec.matrix.org/v1.12/client-server-api/#midentity_server https://spec.matrix.org/v1.12/client-server-api/#mfully_read so definitely needs a clean up. |
|
Using MatrixEvent for account data seems like the world's worst idea, but I can't disagree with your conclusion that fixing it is a job for another day. I was just wondering if we can prepare for that day in any way that would minimise future breaking changes. Anyway, will butt out. |
richvdh
left a comment
richvdh
left a comment
There was a problem hiding this comment.
I applaud the effort, but to make best use of it, it needs better documentation imho
| } | ||
|
|
||
| interface SecretInfo { | ||
| export type SecretInfoKey = Assignable<AccountDataEvents, SecretInfo>; |
There was a problem hiding this comment.
Wouldn't it be easier/clearer/make more sense generally to define SecretInfoKey explicitly, and derive AccountData from that, rather than the other way around?
There was a problem hiding this comment.
That wouldn't be extensible by consumers of this library, only interfaces are extensible via declaration merging.
https://www.typescriptlang.org/docs/handbook/declaration-merging.html
There was a problem hiding this comment.
Even so, it seems like it would be more intuitive to declare:
export interface SecretStorageAccountDataEvents {
"m.cross_signing.master": SecretInfo;
"m.cross_signing.self_signing": SecretInfo;
"m.cross_signing.user_signing": SecretInfo;
// etc
}
... and then derive AccountDataEvents and SecretStorageKey from that, rather than messing about with Assignable.
But it's not a strong opinion.
| /// The event type storing the user's individual policies. | ||
| /// | ||
| /// Exported for testing purposes. |
There was a problem hiding this comment.
I don't think /// works for ts doc?
There was a problem hiding this comment.
This is just copied out of invites-ignorer.ts verbatim to avoid an import cycle
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
richvdh
changed the title
| * @returns The contents of the given account data event | ||
| */ | ||
| public getAccountData(eventType: string): MatrixEvent | undefined { | ||
| public getAccountData<K extends keyof AccountDataEvents>(eventType: K): MatrixEvent | undefined { |
There was a problem hiding this comment.
Extending this PR to cover fetching account data is fine, but it's a significant scope creep. Adding it in the middle of a review cycle doesn't make a reviewer's job any easier. It'd be good just to make another PR next time.
| } | ||
|
|
||
| interface SecretInfo { | ||
| export type SecretInfoKey = Assignable<AccountDataEvents, SecretInfo>; |
There was a problem hiding this comment.
Even so, it seems like it would be more intuitive to declare:
export interface SecretStorageAccountDataEvents {
"m.cross_signing.master": SecretInfo;
"m.cross_signing.self_signing": SecretInfo;
"m.cross_signing.user_signing": SecretInfo;
// etc
}
... and then derive AccountDataEvents and SecretStorageKey from that, rather than messing about with Assignable.
But it's not a strong opinion.
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
… t3chguy/types-account-data
3 participants
Requires element-hq/element-web#28752