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.

Sign up for GitHub

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

Jump to bottom

maybeStep can also fail with an exception #873

Closed
mitar opened this issue Nov 23, 2018 · 3 comments
Closed

maybeStep can also fail with an exception #873

mitar opened this issue Nov 23, 2018 · 3 comments

Comments

@mitar
Copy link

mitar commented Nov 23, 2018

Issue details

During rebase on the server I am using maybeStep like it is used in collab module, but I am observing that maybeStep can also fail with an exception:

RangeError: Position 235 out of range
2018-11-23_07:18:19.48518     at Function.resolve (/bundle/programs/server/npm/node_modules/prosemirror-model/dist/index.js:945:55)
2018-11-23_07:18:19.48519     at Function.resolveCached (/bundle/programs/server/npm/node_modules/prosemirror-model/dist/index.js:968:60)
2018-11-23_07:18:19.48519     at Node.resolve (/bundle/programs/server/npm/node_modules/prosemirror-model/dist/index.js:1239:70)
2018-11-23_07:18:19.48520     at Node.replace$1 [as replace] (/bundle/programs/server/npm/node_modules/prosemirror-model/dist/index.js:1194:23)
2018-11-23_07:18:19.48520     at Function.fromReplace (/bundle/programs/server/npm/node_modules/prosemirror-transform/dist/index.js:458:30)
2018-11-23_07:18:19.48521     at ReplaceStep.apply (/bundle/programs/server/npm/node_modules/prosemirror-transform/dist/index.js:482:23)
2018-11-23_07:18:19.48521     at Transform.maybeStep (/bundle/programs/server/npm/node_modules/prosemirror-transform/dist/index.js:345:21)

I find this surprising.

Steps to reproduce

I have not yet been able to come up with the reproduction. I am currently observing this in our logs and investigating.

ProseMirror version

[email protected]
[email protected]
[email protected]
[email protected]
@marijnh
Copy link
Member

marijnh commented Nov 23, 2018

Yes, this is by design. The method expects the step to be in the same coordinate space as the document you're trying to apply it to—you can't just try to apply any step to any document. The checks during application are about the structure of the step fitting into the document, not about the positions it refers to being valid for that document. I think you have some other bug where the steps are being mapped incorrectly (or not being mapped when they should be).

@marijnh marijnh closed this as completed Nov 23, 2018
@mitar
Copy link
Author

mitar commented Nov 23, 2018

Thanks for confirming this. I will dig deeper.

@mitar
Copy link
Author

mitar commented Nov 24, 2018

I found a reproduction: #874

jgravois added a commit to showrunner/prosemirror-collab that referenced this issue Mar 15, 2024
@jgravois
account for possible step application failures during rebase
1d2dad9 
we intermittently see steps which can be applied cleanly locally,
but induce an error when this plugin attempts to rebase them on top of
remote confirmed steps.

i am aware that the possibility of `maybeStep()` 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, this uncaught error occurs for us with orders of magnitude more
frequency in [email protected] than it does in [email protected]
jgravois added a commit to showrunner/prosemirror-collab that referenced this issue Mar 15, 2024
@jgravois
account for possible step application failure during rebase
c33e909 
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 [email protected] than [email protected]
@jgravois jgravois mentioned this issue Mar 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@marijnh @mitar