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

fix remove infinite loop #13

Merged

Conversation

tanhauhau
Copy link
Contributor

@tanhauhau tanhauhau commented Nov 10, 2019

when adding the this.remove(), I tried to make the index in the enter() "consistent", as if the original index before any node being removed in the array.

However, little did I know that, if you try to remove more than one node in the node array, it will cause an infinite loop, because the subsequent index i use to splice the array is off by 1.

i wonder what is your thoughts on this,

  • to have the index in the enter() to be the original index
  • or the updated index after splice ?

@Rich-Harris
Copy link
Owner

Oh, that's tricky... IIRC the original motivation for including the index was so that you could remove nodes yourself. Now that this.remove() is a thing (and can be implemented more reliably than it can be in userland) maybe we just shouldn't pass the index down?

Then again, there's one place in the Svelte codebase where the index is actually being used to insert nodes immediately after the current node. Short of adding an API for that (which would be pretty niche), I daresay that's an argument for index being the current index and not the original one. It also makes it easier to answer questions like 'is this the last node in the array?'

@Rich-Harris
Copy link
Owner

Opened an alternative PR, branched off of this one, that uses the current index rather than the original: #18

@tanhauhau
Copy link
Contributor Author

I guess you are right, there's no hard reason why the index must be the original index.

@Rich-Harris Rich-Harris merged commit 4861abf into Rich-Harris:master Feb 3, 2020
@Rich-Harris
Copy link
Owner

Released 2.0.1

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.

2 participants