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

Redaction and change events for relations #911

Merged
merged 8 commits into from
May 13, 2019
Merged

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented May 10, 2019

Reviewer: Each commit in this PR is self-contained and has some additional context in the commit message, so you may want to review commit by commit.

This adds reactions and change event support needed for relations events in the JS SDK, which will be used for reactions and message editing features.

Fixes element-hq/element-web#9485
Part of element-hq/element-web#9572
Part of element-hq/element-web#9574
Used in matrix-org/matrix-react-sdk#2954

@jryans jryans requested a review from a team May 10, 2019 17:26
@turt2live turt2live self-requested a review May 10, 2019 18:16
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

For future PRs, please separate things like 00e4236 due to their size. It's fine for this PR because it's already done though.

I mostly have questions and github doesn't have an option for "this is probably fine but there's a path for more code depending on the answer to some questions".

As an aside: how does this behave if the relation comes in before the event? What if the event is cut off by limited: true on a sync but the relation is not? (not questions to fix for this PR)

.babelrc Outdated Show resolved Hide resolved
src/models/relations.js Show resolved Hide resolved
src/models/relations.js Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

questions answered - thanks :)

@jryans
Copy link
Collaborator Author

jryans commented May 10, 2019

For future PRs, please separate things like 00e4236 due to their size. It's fine for this PR because it's already done though.

Right, sorry... I try to separate when it makes sense to me. In this case, I was worried it would add lag to the review process for the feature.

As an aside: how does this behave if the relation comes in before the event? What if the event is cut off by limited: true on a sync but the relation is not? (not questions to fix for this PR)

Possibly badly, will need to test. 😅 We should be storing any relations as they come in, but haven't tried it yet. In the long run, we'll switch to server-side aggregation instead, which should handle those cases better.

jryans added 7 commits May 13, 2019 13:52
This enables compiler and linting features to allow class properties like we do
in the React SDK.
This watches for redactions of relations and updates the relations collection
to match, including various aggregations. In addition, a redaction event is
emitted on the redaction collection to notify consumers of the change.

Part of element-hq/element-web#9574
Part of element-hq/element-web#9485
…lections

If you ask for relations but none currently exist, we return `null` to avoid the
overhead of many empty relations objects in memory. However, we still want some
way to alert consumers when one _is_ later made, so this event level event
provides that.

Part of element-hq/element-web#9572
Part of element-hq/element-web#9485
This adds a `Relations.add` event that consumers can listen for to be notified
each time an additional relation event is added to a relations collection.

Part of element-hq/element-web#9485
Part of element-hq/element-web#9572
This special cases pending relation events to go directly to the timeline sets
and ignores the `pendingEventOrdering` option. This feels a bit strange in the
code, so we should revisit this choice when we stabilized relation support.
@jryans jryans force-pushed the jryans/aggregations branch from 3e02fea to f411d50 Compare May 13, 2019 13:17
To avoid an O(n^2) situation with every relations container trying to process
every redaction that may occur in a room, this switches to an event-level
notification, so that the specific relations container who cares can listen to
just the events it wants to know about.
@jryans jryans merged commit 6a5f5b2 into develop May 13, 2019
@t3chguy t3chguy deleted the jryans/aggregations branch May 10, 2022 14:22
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.

API support for aggregations
2 participants