Don't grow subscribers array indefinitely

[bqqbarbhg]

When removing subscribers from an entity shrink the _subscribers array instead of just replacing the removed one with null.

For further optimization: When iterating over _subscribers there should not be any null items anymore so checks for that can be omitted if this fix is accepted.

[arnelenero]

Thanks for submitting your PR.

Please check entity.js, in createSetter function. There is an existing cleanup there, which was intentionally deferred to make sure the subscriber callback loop is complete before it removes items from the array.

Do you think moving that to the useEffect cleanup will be better? If I remember correctly, in my prior art (react-entities), it was originally in the effect cleanup, but there was a timing issue that was reported, so I moved it to the current location. This new library is based on that same core, so I figured the fix carried over here.

I also understand that running the cleanup only whenever an unmount happens, as opposed to every update, would have performance benefits. But again, there were reports of timing issues before that seem to break the subscriber loop when a component unmounts before the loop has ended.

One option I have is to use Sets instead of arrays. But it appears arrays perform better in simple for-loops than set's forEach. Yet, there's that possible bottleneck of using array filter to do the cleanup.

I have to review the figures, but up to a certain point, iterating through an array with null items using simple for-loop appears to perform better than the same for-loop combined with a filter call. But once we reach that certain array length, the for-loop starts to get slower. Maybe another option is to do the cleanup only each time the array length reaches a certain threshold. This might be a good balance for performance?

What do you think?

[bqqbarbhg]

Thanks for the detailed reply! That makes sense, in that case this PR is unnecessary.

To deter further low-effort PRs (sorry for this one!) it might be worth it to put a comment to the cleanup function to make it not look like a bug:

// null subscribers are cleaned up later in createSetter()
entity._subscribers[i] = null

Fun related micro-optimization: if you do the filter() before looping through the callbacks in createSetter() you don't need to do the (typeof entity._subscribers[i] === 'function') check before calling them :D

[bqqbarbhg]

And calling filter() less often might have the benefit that the _subscribers identity remains constant which means useEntity() doesn't get called every time as it's listed as a dependency.

With my limited React Hook knowledge I'd say it might be worth to look into removing entity._subscribers from useEffect() dependencies as it's mutable and it's identity does not have a semantic effect on that effect. In the current state it looks akin to passing window as a dependency to an effect calling setTimeout()/clearTimeout() as an example.

[arnelenero]

Thanks again @bqqbarbhg for this. It's funny that I used to have a comment there about the null subscribers, prior to initial commit, but I removed it for no good reason at all. LOL. So this PR has been helpful, actually. I need to restore that comment there.

And I apologize that the absence of that comment has raised this red flag. I agree it looked suspicious code without the proper comment.

[arnelenero]

And calling filter() less often might have the benefit that the _subscribers identity remains constant which means useEntity() doesn't get called every time as it's listed as a dependency.

With my limited React Hook knowledge I'd say it might be worth to look into removing entity._subscribers from useEffect() dependencies as it's mutable and it's identity does not have a semantic effect on that effect. In the current state it looks akin to passing window as a dependency to an effect calling setTimeout()/clearTimeout() as an example.

Actually you made a very good point there. That filter in createSetter used to be a splice. In an effort to make the code look more readable, I think I just made it trigger unnecessary triggering of useEffect. I will revert it to splice so that the _subscribers array remains constant reference-wise. This would then make it safe to keep it as dependency to the useEffect (to keep linting warnings quiet LOL).

Thank a lot!