Implement MSC4429: Profile Updates for Legacy Sync#5246
Conversation
and grammar correction
and rename and comment the functions a bit
and fix bugs with mismatched array/string keys It's not clear why all our key paths are arrays, even for single keys. I suspect someone — probably me — misreading the docs many years ago, but the queries need to match the object store creation.
| * Store user profile details from a sync. Existing profiles will be overwritten. | ||
| * @param userProfileTuples - A set of userIds to profiles. | ||
| */ | ||
| storeUserProfiles(userProfileTuples: Array<[string, SyncUserProfile]>): Promise<void>; |
There was a problem hiding this comment.
ooi why is it an array of tuples vs a record/similar?
There was a problem hiding this comment.
seems like there was no particular reason - I've gone with a map because non-static keys, although it has to be said that when called with a single update this is now the same with the addition of a Map constructor.
| * Store user profile details from a sync. Existing profiles will be overwritten. | ||
| * @param userProfiles - A map of userIds to profiles. | ||
| */ | ||
| storeUserProfiles(userProfiles: Map<string, SyncUserProfile>): Promise<void>; |
There was a problem hiding this comment.
This type referencing unknown makes it very weak. Why are we using unknown in storage? Using unknown for "down sync" as the jsdoc says is fine, if it is then validated and upgraded to a stronger type
| /** | ||
| * An object of extended profile attributes for a user as it arrives down the sync stream. | ||
| */ | ||
| export type SyncUserProfile = Record<string, unknown>; |
There was a problem hiding this comment.
Presumably this isn't truly unknown, given its JSON it can be a much more specific type
There was a problem hiding this comment.
We could move (or copy) https://github.com/element-hq/element-web/blob/develop/apps/web/src/%40types/json.ts to js-sdk which would make it a bit more specific but I think that's about it. For storage, we don't really know much more other than that it's not null or undefined I think.
There was a problem hiding this comment.
Well presumably we do know a lot more, for it to even be storable, given we presumably can't store complex cyclic objects
There was a problem hiding this comment.
The spec is broad https://spec.matrix.org/v1.17/client-server-api/#profiles allowing any JSON type - so yeah possibly we should copy the types from EW
There was a problem hiding this comment.
Done. should be a little less vague now.
and use them to type the user profiles a bit more than just unknown
| @@ -0,0 +1,16 @@ | |||
| /* | |||
| Copyright 2024 New Vector Ltd. | |||
There was a problem hiding this comment.
Inheriting the copyright might be fine but I think we need to give up the license, least because it refers to LICENSE files which do not exist in this repo
| export interface JsonObject { | ||
| [key: string]: JsonObject | JsonArray | JsonValue; | ||
| } | ||
| export type Json = JsonArray | JsonObject; |
There was a problem hiding this comment.
this should probably be JsonDocument
3 participants
matrix-org/matrix-spec-proposals#4429
This implements half functional support for profile updates over legacy sync.
It does not yet fully support storing them over sessions, but does handle updates.Now stores updates in indexeddb with write-through caching on the get API so they'll eventually all be stored in the database even if you turn the feature flag on after your initial sync.
Closes https://github.com/element-hq/wat-internal/issues/464
Checklist
public/exportedsymbols have accurate TSDoc documentation.