-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Real-time collaboration: Add Yjs awareness #72437
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
Conversation
|
Size Change: +20 B (0%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 921f9f9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18594934605
|
921f9f9 to
22de1e2
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
aff74e1 to
5bd7ea0
Compare
packages/core-data/src/entities.js
Outdated
| * @type {Record< string, boolean >} | ||
| */ | ||
| supports: { | ||
| awareness: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is awareness a properly of the entity config. It feels to me like it's something global to the whole sync manager no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is awareness a properly of the entity config.
We anticipated that it may not be desirable to provide presence indicators for some entities (such as tags or font collections). However, that is probably better configured by the sync manager / awareness implementation. There is a small amount of overhead associated with creating an awareness instance, but if it is unused it is neglible.
Removed in 9d3aa45
5bd7ea0 to
84222ee
Compare
| const providerResults = await Promise.all( | ||
| providerCreators.map( ( create ) => | ||
| create( objectType, objectId, ydoc ) | ||
| create( objectType, objectId, ydoc, new Awareness( ydoc ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What still unclear to me is why are we creating Awareness per "objectId". In my mind Awareness was something more global (like a unique instance for the whole app/manager) but I'm probably misunderstanding what awareness holds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great query @youknowriad! An awareness instance holds arbitrary data, is attached to a single Yjs document, and piggypacks on the transport for that Yjs document.
In our plugin, a post entity's awareness instance holds the user's information (for rendering a list of connected peers) and the user's current selection (for rendering cursor presence indicators).
Our assumption was that, over time, we would enhance WordPress Admin so that every mechanism for editing or interacting with an entity would result in real-time collaboration and presence indicators for other interested peers. For example, User 1 might be editing a post using the post editor, User 2 might be editing the same post via "Quick Edit," and User 3 might be editing a synced pattern that is used in that post. Users 1 and 2 should see each other, and also see User 3, via contextually appropriate presence indicators.
Of course, none of this actually requires entity-specific awareness instances; it was just an expedient and natural choice given Yjs's API. Awareness instances require a Yjs document, but that document can easily be empty.
A single global awareness instance would probably be an overwhelming firehose of activity, and would needlessly sync data that would be mostly ignored by individual users. Entity-level awareness instances can also be wasteful: someone using the Quick Edit feature has no need for cursor-level presence indicators, but would receive that data anyway.
We could find a balance; screen- or page-based awareness instances might allow users to see the activity of other users also on that page or screen. However, it would not capture activity from users editing a given entity via a different screen.
Exploring your question has been an interesting exercise, and it makes me think that our awareness implementation is not mature enough to support a stable architectural decision. Revisiting it after implementing some of these hypothetical examples might be beneficial.
We also don't require this change to support our plugin; we can easily create our own awareness instance. Therefore, I'm inclined to close the pull request for now. Let me know if you have additional thoughts, with thanks for the critical eye!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to close to the issue.
I personally do lean towards a single awareness (attached to a dedicated document or empty or something) because a user 1 might be editing entity A and entity B and the same time and it needs to sync with user 2 editing entity A and user 3 editing entity B. So it's a bit weird to me to store the same awareness data in both entity A and entity B.
I think if you think about wp-admin as an SPA, it does make sense for users to be aware of all the users that are "logged in", where they are... (kind of like figma)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you think about wp-admin as an SPA, it does make sense for users to be aware of all the users that are "logged in", where they are... (kind of like figma)
Agreed. To me that points towards (1) a global awareness instance to hold user information and their current "locations"—whether that's URL paths or something else, I'm not sure; and (2) per-entity (or per-something) awareness instances to track entity-specific actions. But we can let our approach to awareness mature a bit and see where it leads us.
Happy to close to the issue.
Closing for now! Thanks again.
|
Closing in favor of a plugin-based approach while we do additional product exploration. Will reopen when we learn more about the core use cases. |
What?
Instantiate and pass a Yjs
Awarenessinstance to the connection creator.Why?
An
Awarenessinstance allows for the implementation of awareness / presence indicators in the UI. These indicators can be provided by Gutenberg in future PRs. Plugins can also provide an implementation by accessing theAwarenessinstance passed to the connection creator function.How?
supports.awarenessflag to indicate awareness support. It is assumed that some entities will always be edited together and will not need a separateAwarenessinstance.Awarenessinstance to the connection creator.Testing Instructions
The
Awarenessinstance is unused. Refer the instructions in this PR for general testing.Testing Instructions for Keyboard
No UI changes in this PR