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

Fix binding === null due to asynchronous creation. Fixes #42 #54

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/plugins/cursor-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ export const createDecorations = (state, awareness, createCursor) => {
if (ystate.snapshot != null || ystate.prevSnapshot != null || ystate.binding === null) {
// do not render cursors while snapshot is active
return DecorationSet.create(state.doc, [])
} else if (ystate.binding.mapping.size === 0) {
// do not render until binding mappings are present
return DecorationSet.create(state.doc, [])
}
awareness.getStates().forEach((aw, clientId) => {
if (clientId === y.clientID) {
Expand Down
108 changes: 64 additions & 44 deletions src/plugins/sync-plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,63 @@ const getUserColor = (colorMapping, colors, user) => {
return /** @type {ColorDef} */ (colorMapping.get(user))
}

class YSyncPluginState {
constructor (yXmlFragment, { colors, colorMapping, permanentUserData }) {
this.type = yXmlFragment
this.doc = yXmlFragment.doc
this.binding = null
this.snapshot = null
this.prevSnapshot = null
this.isChangeOrigin = false
this.colors = colors
this.colorMapping = colorMapping
this.permanentUserData = permanentUserData
}

init (view) {
if (this.binding) {
return this
}
this.binding = new ProsemirrorBinding(this.type, view)
return this
}

destroy () {
if (this.binding) {
this.binding.destroy()
}
this.binding = null
Copy link
Contributor

@ankon ankon Jun 21, 2021

Choose a reason for hiding this comment

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

Mostly for consistency: Wouldn't hurt to also explicitly init this.binding = null in the constructor?

(Given the below this.binding !== null check -- this may actually be needed, but I don't understand enough about the life-cycle here)

Copy link
Author

Choose a reason for hiding this comment

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

Good spot, fixed.

}

apply (tr) {
const change = tr.getMeta(ySyncPluginKey)
if (change !== undefined) {
for (const key in change) {
this[key] = change[key]
}
}
this.isChangeOrigin = change !== undefined && !!change.isChangeOrigin
if (this.binding !== null) {
if (change !== undefined && (change.snapshot != null || change.prevSnapshot != null)) {
// snapshot changed, rerender next
setTimeout(() => {
if (change.restore == null) {
this.binding._renderSnapshot(change.snapshot, change.prevSnapshot, this)
} else {
this.binding._renderSnapshot(change.snapshot, change.snapshot, this)
// reset to current prosemirror state
this.restore = undefined
this.snapshot = undefined
this.prevSnapshot = undefined
this.binding._prosemirrorChanged(this.binding.prosemirrorView.state.doc)
}
}, 0)
}
}
return this
}
}

/**
* This plugin listens to changes in prosemirror view and keeps yXmlState and view in sync.
*
Expand All @@ -75,7 +132,8 @@ const getUserColor = (colorMapping, colors, user) => {
*/
export const ySyncPlugin = (yXmlFragment, { colors = defaultColors, colorMapping = new Map(), permanentUserData = null } = {}) => {
let changedInitialContent = false
const plugin = new Plugin({
const pluginState = new YSyncPluginState(yXmlFragment, { colors, colorMapping, permanentUserData })
return new Plugin({
props: {
editable: (state) => {
const syncState = ySyncPluginKey.getState(state)
Expand All @@ -85,58 +143,21 @@ export const ySyncPlugin = (yXmlFragment, { colors = defaultColors, colorMapping
key: ySyncPluginKey,
state: {
init: (initargs, state) => {
return {
type: yXmlFragment,
doc: yXmlFragment.doc,
binding: null,
snapshot: null,
prevSnapshot: null,
isChangeOrigin: false,
colors,
colorMapping,
permanentUserData
}
return pluginState
},
apply: (tr, pluginState) => {
const change = tr.getMeta(ySyncPluginKey)
if (change !== undefined) {
pluginState = Object.assign({}, pluginState)
for (const key in change) {
pluginState[key] = change[key]
}
}
Comment on lines -102 to -107
Copy link
Author

Choose a reason for hiding this comment

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

I have removed this, I'm unsure of the larger impact, but everything seems to work fine without it in my project?

Copy link
Member

Choose a reason for hiding this comment

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

Other plugins might use this to set properties like snapshot or colorMapping. We can't just remove this.

Copy link
Author

Choose a reason for hiding this comment

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

@dmonad I have restored this, but dropped the line pluginState = Object.assign({}, pluginState)

I guess this is to make the plugin state immutable? In the example I copied this pattern from, it looks like the plugin apply step returns a new instance of the state to achieve this immutability.

However with the asynchronous setting of the binding using the view, I'm not 100% sure how to achieve this, anybody have any suggestions?

// always set isChangeOrigin. If undefined, this is not change origin.
pluginState.isChangeOrigin = change !== undefined && !!change.isChangeOrigin
if (pluginState.binding !== null) {
if (change !== undefined && (change.snapshot != null || change.prevSnapshot != null)) {
// snapshot changed, rerender next
setTimeout(() => {
if (change.restore == null) {
pluginState.binding._renderSnapshot(change.snapshot, change.prevSnapshot, pluginState)
} else {
pluginState.binding._renderSnapshot(change.snapshot, change.snapshot, pluginState)
// reset to current prosemirror state
delete pluginState.restore
delete pluginState.snapshot
delete pluginState.prevSnapshot
pluginState.binding._prosemirrorChanged(pluginState.binding.prosemirrorView.state.doc)
}
}, 0)
}
}
return pluginState
return pluginState.apply(tr)
}
},
view: view => {
const binding = new ProsemirrorBinding(yXmlFragment, view)
const { binding } = pluginState.init(view)
// Make sure this is called in a separate context
setTimeout(() => {
binding._forceRerender()
view.dispatch(view.state.tr.setMeta(ySyncPluginKey, { binding }))
view.dispatch(view.state.tr.setMeta(ySyncPluginKey, { forceUpdate: true }))
}, 0)
return {
update: () => {
const pluginState = plugin.getState(view.state)
if (pluginState.snapshot == null && pluginState.prevSnapshot == null) {
if (changedInitialContent || view.state.doc.content.findDiffStart(view.state.doc.type.createAndFill().content) !== null) {
changedInitialContent = true
Expand All @@ -145,12 +166,11 @@ export const ySyncPlugin = (yXmlFragment, { colors = defaultColors, colorMapping
}
},
destroy: () => {
binding.destroy()
pluginState.destroy()
}
}
}
})
return plugin
}

/**
Expand Down