Fix binding === null due to asynchronous creation. Fixes #42
#54
Conversation
| if (change !== undefined) { | ||
| pluginState = Object.assign({}, pluginState) | ||
| for (const key in change) { | ||
| pluginState[key] = change[key] | ||
| } | ||
| } |
There was a problem hiding this comment.
I have removed this, I'm unsure of the larger impact, but everything seems to work fine without it in my project?
There was a problem hiding this comment.
Other plugins might use this to set properties like snapshot or colorMapping. We can't just remove this.
There was a problem hiding this comment.
@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?
whawker
force-pushed
the
fix-binding-null
branch
from
732a76f to
9dfa209
Compare
| if (this.binding) { | ||
| this.binding.destroy() | ||
| } | ||
| this.binding = null |
There was a problem hiding this comment.
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)
whawker
force-pushed
the
fix-binding-null
branch
from
9dfa209 to
88430e4
Compare
|
@dmonad We're relying on this fix (currently running on a fork of y-prosemirror). Is there a chance to get this PR merged into |
|
@dmonad Any chance you could take a look over this? |
|
Can also confirm that this fixes an issue related to the binding null I was having. |
There was a problem hiding this comment.
Thanks @whawker for providing a fix.
However, this PR will definitely break other people's code. The y-sync plugin state is supposed to be an Object (although I agree that it should have been an instance of YSyncState, but that wasn't clear to me at the time when I wrote the code). Also, you break some of the features (e.g. adding/overwriting properties to the y-sync state). Can you please revert the changes that are not necessary for this PR? It should be easy to review for me.
| if (change !== undefined) { | ||
| pluginState = Object.assign({}, pluginState) | ||
| for (const key in change) { | ||
| pluginState[key] = change[key] | ||
| } | ||
| } |
There was a problem hiding this comment.
Other plugins might use this to set properties like snapshot or colorMapping. We can't just remove this.
This PR fixes an issue with the sync plugin where
binding === nullleading to issues when calculated relative/absolute positions.Previously
bindingwas set via an asynchronous transaction (usingsetTimeout) - anything that occurred before the execution of the callback could error.This PR changes the sync plugin to
PluginStatepattern (see here) and uses aninitmethod to pass the view and create the binding.