refactor: Make the plugin self-contained #6
Conversation
| ): AutomergePlugin => { | ||
| const stateField: StateField<Value> = StateField.define({ | ||
| create: () => ({ | ||
| lastHeads: automerge.getHeads(handle.docSync()!), |
There was a problem hiding this comment.
This feels like a bit of a footgun. If a user passes a handle which is not ready yet then this will throw an error. I think we have three options:
- Add a "loading" state to the plugin and then attach a
thenhandle tohandle.doc()which transitions to a "ready" state. In the "loading" state the plugin would not allow input. - Make
automergePluginasyncand wait forhandle.doc().await - Document in big letters that you must make sure the handle is ready before passing it to the plugin.
My preference would be for 2. Thoughts?
There was a problem hiding this comment.
Good catch.
Having a loading state seems to be out of scope for the editor plugin. Developers would want to customize the look of the loading state anyway or integrate with their UI library like react suspense.
I'd be hesitant to make the plugin async as there's no precedent for it. Main alternative - yjs, has a sync plugin. It also makes using wrappers that take extensions in props like https://uiwjs.github.io/react-codemirror/ more difficult.
I think my preference would be on option #3 with a clear error message that directs developers to implement a loading state for the editor. This seems to be the most flexible option in terms of API design.
There was a problem hiding this comment.
Good point on the extension props. I agree with your argument, #3 is probably the best we can do.
There was a problem hiding this comment.
seconding what's being said, as a developer I think not implementing the loading state in the plugin is the right way to go - off the top of my head I can think of two ways one might want to integrate it, either not showing the editor at all while loading or having a CM Compartment that is initially empty but is updated to contain the plugin once the handle is ready.
There was a problem hiding this comment.
@dmaretskyi is adding an error message when passed a DocHandle with an undefined doc something you have capacity for? I think it would be really useful to merge this PR so would love to get that in there.
[automergePlugin(handle, path)]needs to be passed to configure the editor.Docfrom@automerge/automergepackage as well.