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

Allow arbitrary awareness fields in Collaboration plugin #4507

Merged
merged 9 commits into from May 19, 2023

Conversation

attnov
Copy link
Contributor

@attnov attnov commented May 17, 2023

Summary

This change allows arbitrary data (for example 3rd party UUID) to be added to awareness. Right now user name and color is hard coded in Collaboration plugin. The new parameter awarenessData is optional.

Usage example

We can specify our arbitrary data like this:

const awarenessData = {
  uuid: 42,
  foo: 'bar',
);

With the new parameter, we can send down our arbitrary data:

<CollaborationPlugin
  id="id"
  providerFactory={providerFactory}
  shouldBootstrap={shouldBootstrap}
  awarenessData={awarenessData}
/>

@vercel
Copy link

vercel bot commented May 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2023 3:02pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 19, 2023 3:02pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2023
@attnov attnov changed the title Allow arbitrary awareness data in Collaboration plugin Allow arbitrary awareness fields in Collaboration plugin May 17, 2023
Copy link
Collaborator

@ivailop7 ivailop7 left a comment

Choose a reason for hiding this comment

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

Thank you, Attila. @acywatson and @zurfyx . Could you give it a look as well.

@fantactuka
Copy link
Contributor

fantactuka commented May 18, 2023

I don't think this works since map is not json serializeable, should be plain object like remaining of awareness data. Personal opinion: naming looks a bit odd, as usage will be awareness.getLocalState().awarenessFields, maybe data or extraData would do better, no strong opinion tho.

Also can add a unit test (see Collaboration.test), as collab is quite sensitive for changes, so it's better to get it covered

Copy link
Contributor

@fantactuka fantactuka left a comment

Choose a reason for hiding this comment

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

~

@attnov
Copy link
Contributor Author

attnov commented May 18, 2023

I don't think this works since map is not json serializeable, should be plain object like remaining of awareness data. Personal opinion: naming looks a bit odd, as usage will be awareness.getLocalState().awarenessFields, maybe data or extraData would do better, no strong opinion tho.

Also can add a unit test (see Collaboration.test), as collab is quite sensitive for changes, so it's better to get it covered

@fantactuka Thanks for the review! I have changed the type from Map to object and added unit test.

Comment on lines 305 to 307
expect(client1.awareness.getLocalState().awarenessFields).toEqual(
awarenessFields1,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use awareness.getStates() instead, to ensure that client1 has proper awareness from client2 and vise versa?

Copy link
Contributor Author

@attnov attnov May 19, 2023

Choose a reason for hiding this comment

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

It looks like here the mock for getStates() only inserts the current client's awareness state. That's why I opted for the getLocalState(). However, I have manually tested and between two collaborating users the awareness is sync correctly.

@@ -225,27 +229,41 @@ export function useYjsFocusTracking(
provider: Provider,
name: string,
color: string,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
awarenessFields?: object,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can use { ... } here and remove eslint comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we don't need the eslint comments anymore - any is not used anymore. Removed all of them.

@fantactuka
Copy link
Contributor

Thanks for updates!

@acywatson @zurfyx thoughts on naming?

@acywatson
Copy link
Contributor

Thanks for updates!

@acywatson @zurfyx thoughts on naming?

I like having awareness in the name, but fields is probably wrong. "awarenessData" or "extraAwaresnessData" seems fine.

doc block on the interface would help people know what it's for.

@attnov
Copy link
Contributor Author

attnov commented May 19, 2023

Thanks for updates!
@acywatson @zurfyx thoughts on naming?

I like having awareness in the name, but fields is probably wrong. "awarenessData" or "extraAwaresnessData" seems fine.

doc block on the interface would help people know what it's for.

Renamed the parameter to awarenessData and added documentation on the inferface.

@@ -62,7 +63,8 @@ class Client {
_listeners: Map<string, Set<(data: unknown) => void>>;
_updates: Uint8Array[];
awareness: {
getLocalState(): void;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
getLocalState(): any;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this better? Besides the object prop what other types we don't know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added types for getLocalState(), getStates(), setLocalState(), etc.
Thanks for checking!

): void {
provider.awareness.setLocalState({
anchorPos: null,
awarenessData: awarenessData,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
awarenessData: awarenessData,
awarenessData,

Similar one below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the change.

@zurfyx zurfyx merged commit 8ed0e0a into facebook:main May 19, 2023
34 of 45 checks passed
vuamitom pushed a commit to bitbriks/lexical that referenced this pull request May 19, 2023
Co-authored-by: Attila Novak <anovak8@bloomberg.net>
This was referenced May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants