Fix unwatch multiple signals #26
Conversation
| const lastIdx = node.producerNode!.length - 1; | ||
| node.producerNode![i] = node.producerNode![lastIdx]; | ||
| node.producerIndexOfThis[i] = node.producerIndexOfThis[lastIdx]; |
There was a problem hiding this comment.
this stuff can be skipped if lastIdx === i
There was a problem hiding this comment.
You're correct, but I believe it won't make any difference. I prefer to implement only the necessary changes needed to fix the issue, making it easier for the owner to review.
There was a problem hiding this comment.
I certainly appreciate that! I also think it would make this code path slightly easier to read if the special case where the producer is the last item in the array was more clearly delineated like it is below the linked snippet.
| expect(notify).toHaveBeenCalledTimes(expectedNotifyCalls); | ||
|
|
||
| watcher.watch(); | ||
| } |
There was a problem hiding this comment.
i might just add expect(expectedNotifyCalls).toBe(3) at the end here just to be clear.
There was a problem hiding this comment.
I appreciate the suggestion, but I think it's not really necessary, considering the logic and other checks.
|
@littledan yeah this is good to land I think. The other PR should be closed, it adds some dubious sanity checks but doesn't fix any bugs as far as I can tell (actually it seems to add code that would swallow legitimate errors in some cases). |
|
@ds300 Just to clarify, I've tested these changes thoroughly, and they don't introduce any new issues. These sanity checks are not meant to hide real errors but to prevent the feature from breaking when it encounters unusual situations. |
Fixes #2
The issue arises when removing an element, which shifts the last element to the current index. As a result, the last index goes out of bounds, and the moved element remains unprocessed.
To address this, iterate backward so that the removed element is replaced with a checked element, while the others remain unchanged.