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

fixed nodesBetween when child is undefinded bug #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XuXuDev
Copy link

@XuXuDev XuXuDev commented Mar 21, 2019

I came across a situation where the child node is undefined.

@marijnh
Copy link
Member

marijnh commented Mar 21, 2019

Could it be that something was calling the method with a to bigger than the node's content size? Because that's the only way it seems this will happen, and that's an error on the caller side, so I'd rather not paper it over by adding null checks here.

@XuXuDev
Copy link
Author

XuXuDev commented Mar 22, 2019

image
My problem is this: i want use rangeHasMark function but this Happening error on nodesBetween

@marijnh
Copy link
Member

marijnh commented Mar 22, 2019

I think the problem is that the positions that rangeHasMark expects are local to the node, and you're passing document-level positions. I.e. .rangeHasMark(0, node.content.size, ...) will probably work better.

@XuXuDev
Copy link
Author

XuXuDev commented Mar 22, 2019

image
image

I took your advice, but I still have problems.

@hubgit
Copy link
Contributor

hubgit commented Jul 10, 2019

Could ProseMirror perhaps output a different error message if the to value is larger than the node's content size?

@marijnh
Copy link
Member

marijnh commented Jul 10, 2019

Raising an explicit error would be fine by me. Do you want to create a pull request?

@anarchang
Copy link

I have also run into the bug in two separate instances. One of the cases is when trying to remove more than one row or column in a table. I had a slightly different solution but I think either would work. My solution adds an extra end case to the for loop:

    for (let i = 0, pos = 0; pos < to && i < this.content.length; i++) {

I am happy to create a PR if you would like to see a different solution.

@marijnh
Copy link
Member

marijnh commented Mar 12, 2020

@anarchang I'm not sure which code that line is from, and what problem you're addressing.

@davidstrouk
Copy link

@marijnh could we merge this PR? It would be useful to raise an explicit error when child is undefined. I don't mind opening a new PR if you think this one is already old.

@marijnh
Copy link
Member

marijnh commented Jan 4, 2024

No. I don't want to paper over stuff that's going wrong elsewhere by silently ignoring it here.

@davidstrouk
Copy link

Ok, I understand. Could we at least raise an exception so that we can catch it elsewhere? TypeError seems to broad in order to catch it.

I know it was a while ago, but you wrote

Raising an explicit error would be fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants