prosemirror/prosemirror-collab
1
0
Fork 0
Code Pull requests Activity

account for possible step application failure during rebase #7

Closed
jgravois wants to merge 1 commit from more-defensive into master
pull from: more-defensive
merge into: prosemirror:master
Conversation 1 Commits 1 Files changed 1 +9 -3
jgravois commented 2024-03-22 18:05:44 +01:00 (Migrated from github.com)
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

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](https://github.com/ProseMirror/prosemirror/issues/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' :upside_down_face: 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 commented 2024-03-23 08:21:57 +01:00 (Migrated from github.com)
Copy link

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.

> 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.

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
prosemirror/prosemirror-collab!7
No description provided.