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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

account for possible step application failure during rebase #7

Closed
wants to merge 1 commit into from

Conversation

jgravois
Copy link

hi @marijnh 馃憢

we intermittently see steps which can be applied cleanly locally, but induce an error when rebased on top of remote confirmed steps.

i am aware that maybeStep() possibly throwing is by design. locally i'm seeing desirable behavior with the patch here, which simply ignores the failed steps altogether.

things i'm unsure of:

  1. does more need to be done to purge these failed steps from any other unconfirmed steps that may be present?
  2. is there something i could be doing outside of the collab plugin to make these steps more invertable/rebasable?

i haven't been able to isolate a simplified repro case for this error yet but i'm happy to describe the scenario in more detail if it would be helpful. the short version is (unsurprisingly), 'its complicated' 馃檭

fwiw, with the same code, we're seeing this error orders of magnitude more frequently in prosemirror-transform@1.8.0 than prosemirror-transform@1.4.2

we intermittently see steps which can be applied cleanly locally,
but induce an error when rebased on top of remote confirmed steps.

i am aware that `maybeStep()` possibly throwing is [by design](ProseMirror/prosemirror#873). locally
i'm seeing desirable behavior with the patch here, which simply
ignores the failed steps altogether.

things i'm unsure of:
1. does more need to be done to purge these failed steps from any other
unconfirmed steps that may be present?
1. is there something i could be doing outside of the collab plugin to
make these steps more invertable/rebasable?

i haven't been able to isolate a simplified repro case for this error yet
but i'm happy to describe the scenario in more detail if it would be helpful.
the short version is (unsurprisingly), 'its complicated' 馃檭

fwiw, with the same code, we're seeing this error orders of magnitude more
frequently in prosemirror-transform@1.8.0 than prosemirror-transform@1.4.2
@marijnh
Copy link
Member

marijnh commented Mar 23, 2024

we intermittently see steps which can be applied cleanly locally, but induce an error when rebased on top of remote confirmed steps.

This suggests there is a problem in the way you are collecting or rebasing these steps. This patch is definitely not the solution. Code that silently swallows errors here is not something I'm going to merge.

@marijnh marijnh closed this Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants