Skip to content

Conversation

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Mar 22, 2021

I created a test that reproduced the issue on #52 correctly expands node with 100.000 children
and the implementation fixed the issue.

Turns out that when chunking the nodes, the two arguments for splice (start and delete) were not taken into account and they add up from chunk to chunk making the subsequent start off by 2*orderPartsCursor(good thing I did the test with 100k nodes and not 40k)

One more thing, I ended up adding one extra test correctly collapses node with 100.000 children but since there was no issue with collapsing I didn't see this one fail. Let me know if you want me to remove it.

closes #52

@sirgallifrey
Copy link
Contributor Author

I also tested on the storybook reproduction and it is working fine after this fix :)

@Lodin
Copy link
Owner

Lodin commented Jul 10, 2021

Hey @sirgallifrey, I finally found some time to get through that ticket again. I have a slightly smaller solution without orderPartsCursor variable in the fix/max-args-bug branch. Since it is very similar to your solution, and I also used your tests (with small updates), I would like to respect your contribution. So if you update that PR with the changes from the branch I mentioned above, I will merge the PR.

Sorry again for making you wait for so long.

@sirgallifrey
Copy link
Contributor Author

sirgallifrey commented Jul 13, 2021

@Lodin no need to be sorry, I understand you are donating you time for free to this project to the benefit of everyone. also the workaround I found allowed me to use it despite the problem :)

Thanks for allowing me to merge the PR, I appreciate it. Now I'm the one with little time, but probably I'll have some moments later this week to update it.

@sirgallifrey
Copy link
Contributor Author

sirgallifrey commented Jul 24, 2021

@Lodin can you have a look? I believe our branches are identical now

Copy link
Owner

@Lodin Lodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Lodin Lodin merged commit d29cb9e into Lodin:master Jul 24, 2021
@sirgallifrey sirgallifrey deleted the fix/33k-children-issue-52 branch July 24, 2021 20:28
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.

Bug while expanding nodes with more than 32766 children - v3

2 participants