-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Real-time Collaboration: Add hook for accessing awareness data #75009
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
|
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. |
|
Size Change: +283 B (+0.01%) Total Size: 2.98 MB
ℹ️ View Unchanged
|
| @@ -41,9 +54,71 @@ export class PostEditorAwareness extends BaseAwarenessState< PostEditorState > { | |||
| public setUp(): void { | |||
| super.setUp(); | |||
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.
The hasSetup property is needed to make this call idempotent and avoid setting up listeners multiple times. There was a bug previously because the value was never updated, can you address that?
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.
Ah, that's the bug I misunderstood. Lemme look into it.
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 I've done is:
- Have the
setupfunction in the base awareness class do thehasSetupRun check. It won't re-do the setup if its already done. - Create a new
onSetupfunction that implementors can override based on what they want. It'll be called within setup and protected by the guard itself. Now, implementors don't have to duplicate thehasSetupRuncheck as it's already handled in thesetupmethod that they don't call or override.
I'm going to attempt a follow up PR in which I'm going to see if its possible to move the setup into the loadEntity and add guards within the onSetup to only execute once a UI component is using it. This will definitely be tricky and may not even be the right approach. But all this to say that a follow up PR can improve this.
chriszarate
left a comment
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.
Can you remove the notifications bit and PR it separately? It needs a bit more thought.
Co-authored-by: Chris Zarate <chris.zarate@automattic.com>
…for-using-awareness-data
…lementation checks to an onSetup
| /** | ||
| * Get the awareness instance for the given object type and object ID, if supported. | ||
| * | ||
| * @template {AwarenessState<any>} State |
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.
Does the linting complain about this? Just curious since its redundant with the generic type annotation
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.
Yip, I got a complaint about this when I tried to lint the file. It said it didn't know what State was, and Claude suggested that template would be a good way to fix it
chriszarate
left a comment
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.
Make sure to additionally add props for Shekhar and Alec
* Add a new hook for awareness data * Export the hook as a private api * Add in notifications for when the post is updated, and a method to get debug data * Update the readme * Update package-lock and ensure that the exported type is documented * Update packages/sync/src/index.ts Co-authored-by: Chris Zarate <chris.zarate@automattic.com> * Remove the exports, notices and update the README * Add in the hasSetup check to prevent multiple setups and move the implementation checks to an onSetup * Make the onsetup abstract and make setup readonly --------- Co-authored-by: Chris Zarate <chris.zarate@automattic.com> Co-authored-by: ingeniumed <ingeniumed@git.wordpress.org> Co-authored-by: alecgeatches <alecgeatches@git.wordpress.org> Co-authored-by: shekharnwagh <shekharnwagh@git.wordpress.org>
What?
Expose Awareness data for consumption within UI components.
Why?
UI components will be added into the editor package that'll need information from the Awareness instance.
How?
This react hook will allow access to the data within the
PostEditorAwarenessStateinstance, without exposing any of the underlying Yjs details. Exposing any of those details would leak the doc, which would be problematic for the the real-time collaboration experience.As a result, as data is added in awareness implementations it'd be exposed via this hook.
This hook is exposed via a private API to ensure it's available only within Gutenberg. Also, there is an unused
getDebugDatamethod within thePostEditorAwarenesssStateimplementation. This is for the future when debug data related to the RTC experience would be exported.Testing Instructions
editorpackage to consume this new hook. I'd recommend justconsole.loggingtheactiveUsersfor example within thedocument-bar.Testing Instructions for Keyboard
None
Screenshots or screencast
The data being
console.loggedA new notification when a post is saved by a collaborator