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

Don't mutate deepMap values #305

Merged
merged 1 commit into from
May 16, 2024
Merged

Conversation

russelldavis
Copy link

Previously, calling setKey on a $deepMap would mutate the value in-place. This can cause various problems, for example #250 and #290. Those were both solved with workarounds, however, this solves it in a better way, by not mutating the deepMap in the first place.

Some advantages of this approach:

  • No need for structuredClone
  • The oldValue passed to listeners is the same object reference as the value that was previously passed. This is how immutable stores usually work, so it will align better with expectations, cause fewer issues, and allow for new use cases, like the upcoming batching PR which needs that behavior.
  • Copies were already being made at each nesting level of the key being set, they just weren't being used (so maybe this was the original intent?). Either way, that means there's no new work being done, and perf will actually improve since we no longer need to call structuredClone on oldValue.

As part of this change, when a key path extends past the end of an array the result will be a sparse array rather than filling it with undefined. It would have required extra code (and slower perf) to keep the old behavior, and I don't see a good reason for it. The new behavior aligns with what JS does natively, and with other stores like SolidJS. But let me know if that's a problem.

@ai
Copy link
Member

ai commented May 16, 2024

Can you also explain a little more how do you do it? (Sorry, I am full in another project right now for deep review)

Previously, calling setKey on a $deepMap would mutate the value
in-place. This can cause various problems, for example nanostores#250 and nanostores#290.
Those were both solved with workarounds, however, this solves it in a
better way, by not mutating the deepMap in the first place.

Some advantages of this approach:
* No need for structuredClone
* The oldValue passed to listeners is the same object reference as the
value that was previously passed. This is how immutable stores usually
work, so it will align better with expectations, cause fewer issues, and
allow for new use cases, like the upcoming batching PR which needs that.
* Copies were already being made at each nesting level of the key being
set, they just weren't being used (so maybe this was the original
intent?). Either way, that means there's no new work being done, and
perf will actually improve since we no longer need to call
structuredClone on oldValue.

As part of this change, when a key path extends past the end of an array
the result will be a sparse array rather than filling it with undefined.
It would have required extra code (and slower perf) to keep the old
behavior, and I don't see a good reason for it. The new behavior aligns
with what JS does natively, and with other stores like SolidJS. But let
me know if that's a problem.
@russelldavis
Copy link
Author

It all happens in setByKey. It uses a copy of each object/array at each level of the key path, so that nothing in the original object tree is ever mutated. It's like making a deep copy, but only on the parts of the object tree that will be mutated (the rest is a shallow copy).

@ai
Copy link
Member

ai commented May 16, 2024

So if we change a.b key, it means what will be mutated only?

{ // mutates
  a: { // mutates
    b: 'new value', // mutates
    c: {} // copies
  },
  d: {} // copies
}

@russelldavis
Copy link
Author

Yeah, so in your example, after you call $store.setKey("a.b", "foo"), $store.value, $store.value.a and $store.value.a.b will be new objects, while $store.value.a.c and $store.value.d will remain as the same unmodified objects.

@ai ai merged commit e0d91c3 into nanostores:next May 16, 2024
4 checks passed
@russelldavis
Copy link
Author

Thanks for merging. cc @dkzlv @euaaaio for awareness

russelldavis added a commit to russelldavis/nanostores that referenced this pull request May 18, 2024
ai pushed a commit that referenced this pull request May 18, 2024
ai pushed a commit that referenced this pull request Jun 25, 2024
Previously, calling setKey on a $deepMap would mutate the value
in-place. This can cause various problems, for example #250 and #290.
Those were both solved with workarounds, however, this solves it in a
better way, by not mutating the deepMap in the first place.

Some advantages of this approach:
* No need for structuredClone
* The oldValue passed to listeners is the same object reference as the
value that was previously passed. This is how immutable stores usually
work, so it will align better with expectations, cause fewer issues, and
allow for new use cases, like the upcoming batching PR which needs that.
* Copies were already being made at each nesting level of the key being
set, they just weren't being used (so maybe this was the original
intent?). Either way, that means there's no new work being done, and
perf will actually improve since we no longer need to call
structuredClone on oldValue.

As part of this change, when a key path extends past the end of an array
the result will be a sparse array rather than filling it with undefined.
It would have required extra code (and slower perf) to keep the old
behavior, and I don't see a good reason for it. The new behavior aligns
with what JS does natively, and with other stores like SolidJS. But let
me know if that's a problem.
ai pushed a commit that referenced this pull request Jun 25, 2024
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.

None yet

2 participants