-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Bootstrap a minimal sync package #52681
Conversation
Size Change: +440 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
Flaky tests detected in 8bea6e8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5610713375
|
const docName = `${ objectType }-${ objectId }`; | ||
new WebrtcProvider( docName, doc, { | ||
// @ts-ignore | ||
password: window.__experimentalCollaborativeEditingSecret, |
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.
It would be good to define how security will work.
If we have a single password for all the documents on the site, how do we ensure an author or a less privileged user does not access (or edits and propagates to admin peers) entities for which that role does not have access?
If we have a password per document, how is the password generated and propagated to peers for how long that password is valid and how a new password is generated/stored? If I remove a role or capability from a user how do we make sure the previous password the user may have is not valid anymore?
A third option is we have a password for all the documents on a site. And the server maps peerid, to role/capabilities (e.g.: during the signaling) peers have access to this map from peerid to capabilities and when a peer receives updates from another peer would validate if the updates are valid or not. More complex involves changing yjs to add a layer where peer updates may be discarded.
I'm not sure which option is best and I don't think it is a blocker for now but it is something we need to decide.
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.
Of course, this is just a very naive thing to get things rolling. That's nothing close to a real solution here.
*/ | ||
export function connectWebRTC( objectId, objectType, doc ) { | ||
const docName = `${ objectType }-${ objectId }`; | ||
new WebrtcProvider( docName, doc, { |
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.
We are creating a connection per document, and the document has "edited", and "base" (each entity has two documents), so with 3 users collaborating, and assuming the site editor loads an average of 7 entities (it can easily be more) we have 7x2x3= 42 socket connections. I'm not sure if there are limits on the number of websocket connections the browser can keep open but I guess it is something we should keep an eye on.
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.
From what I've read on yjs, that seems like a common approach but of course, it's something we should check.
*/ | ||
export function connectWebRTC( objectId, objectType, doc ) { | ||
const docName = `${ objectType }-${ objectId }`; | ||
new WebrtcProvider( docName, doc, { |
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.
On non-localhost hosts, there is a crash here and the editor stays white:
crypto.js:17 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'importKey')
at Module.deriveKey (crypto.js:17:1)
at new WebrtcProvider (y-webrtc.js:586:27)
at connectWebRTC (connect-webrtc.js:22:2)
at Object.bootstrap (provider.js:74:10)
at async resolvers.js:85:5
at async index.js:590:6
It is possible to reproduce this with gutenberg.run I'm not sure yet why the crash happens yet and I think it is something we need to fix.
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.
Maybe because of the default signaling server?
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.
On the site editor, I'm not being able to save the editor never gets "dirty". On post editor things work well.
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.
Global style changes don't propagate to the other peers e.g.:when a background color is applied. It is a problem with global styles where state is used and we don't have an edited object in the entity for global styles as it happens with blocks. It will need a refactor outside of the scope of this PR.
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.
Awesome work @youknowriad it is impressive how things work so smoothly and multiple users can already collaborate 👍
I think we should merge this soon as it is bewind an experiment. The things we would ideally fix before the merge are an empty editor on non-localhost hosts, and not being possible to save on the site editor.
* @return {Promise<() => void>} Promise that resolves when the connection is established. | ||
*/ | ||
export function connectWebRTC( objectId, objectType, doc ) { | ||
const docName = `${ objectType }-${ objectId }`; |
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 the document names strings should be site specific e.g.: each site has its own unique document names. It would leave the possibility open for example to use a y-websocket connector for multiple WordPress websites, or a y-webrtc signaling server for multiple sites, or even a public .org signaling server.
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.
Makes sense
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.
This is behind an experiment I don't see a reason for not merging it and continuing the iterations.
Now that node is upgraded on trunk, I've updated this PR to target trunk and rebased it. |
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/synchronization.php ❔ lib/experimental/editor-settings.php ❔ lib/experiments-page.php ❔ lib/load.php |
Ok let's merge this one. It's far from ready or usable but it's a placeholder PR that will allow developers to iterate on this in parallel. Everything is behind an experimental flag. |
Related #52593
What?
The goal of this PR is to introduce a new package
@wordpress/sync
that will serve as a playground for us to iterate on the synchronization and live collaboration features. For the details about the architecture and the reasoning behind this package, please refer to this post https://make.wordpress.org/core/2023/07/13/real-time-collaboration-architecture/It's most likely going to take a lot of time and iterations for us to arrive a decent API and experience for the sync engine and live collaboration, so it's important to have a starting point and a place to iterate without impacting the other work happening on the project, for this reason, all the features introduced by this work are hidden behind an experimental flag.
At the moment, the base architecture of the package is in place but it's not entirely hooked into all the necessary places:
Testing Instructions
1- Upgrade node
2- Enable the feature flag
3- you can open the same post in two different tabs and notice that changes propagate.