fix: more efficient and correct reactive set#11967
Conversation
🦋 Changeset detectedLatest commit: e9bb256 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
So, we have chosen to ignore the fact that I feel that this is something that could be addressed without performance concerns if we let define extra teardown logic in the effect implementation. Or would the performance impact of that be more significant than potentially unnecessary calling user code dozens if not thousands of times? |
|
Not ignoring, just optimising for what seem like the common cases — being fine-grained in all cases would take more work and memory |
|
Hi! If you'd like me to open an issue for this I can, I just wanted to mention that I personally am working on something (personal, not work) that places equal importance on in the set vs not, specifically I have a finite number of keys, am doing something reactively based on each of those keys (regardless of if they're in the set or not), and am basically using the |
I reverted #11946 because I don't think it's the right fix. We don't need to read all the sources in a set every time we iterate over the keys — that's very wasteful indeed. Nor, in fact, do we need to add a source for every
set.add(value).The only thing that actually needed to change AFAICT is that we need to increment the version and update the size inside
deletewhenever a value was deleted. Previously we were only doing that when a source already existed.This PR fixes that, and makes things generally a bit simpler and more efficient (no more redundant source creation in
add)Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.Tests and linting
pnpm testand lint the project withpnpm lint