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

Should immutable scope copy the object before passing it to the scope? #23

Closed
barneycarroll opened this issue Jan 25, 2019 · 2 comments
Closed

Comments

@barneycarroll
Copy link
Owner

As suffered by @robinchew on the Meiosis chatroom, Patchinko explicitly (attempts) copies target objects before passing them to scopes. This is irritating, especially as seen in this test case, where the scope seems to be written purposely to avoid that behaviour.

I recall being conflicted about this before and justifying to myself that although this seemed to be taking immutability to bloody-minded levels, that was kind of the point of writing a specifically immutable version of Patchinko - if you don't want that, you can always use the explicit or constant flavours.

But, whenever I think about it, I can't really think of a case where an author would want a scope to be fed a copy. Scopes are designed for the purposes of explicit fine-grained control - the original use case was proxying guards around functions, where keeping the unmodified original reference is often essential. - and even where the author might for whatever reason really need a copy, well they can do that within the scope using Patchinko.

Here's the fixed version, where I simply drop the condition. Yay or nay?

@zkf
Copy link

zkf commented Jul 23, 2019

I ran into this today, with an object containing a Set. The Set is replaced by a new empty Set before the patch function is applied.
Your fixed version corrected the issue.
I can’t really comment on the general correctness of your fix, though.
As a workaround, I’ve substituted Arrays for Sets in my code.

console.log(O({object: {set: new Set([1,2])}}, {
  object: O({
    set: O(oldSet => {
      const s = new Set([...oldSet])
      s.add(3)
      return s
    }),
  }),
}))

// Expected result (“fixed” version of Patchinko):
// { object: { set: Set (3) {1, 2, 3} } }
// Actual result (current version of Patchinko):
// { object: { set: Set (1) {3} } }

@barneycarroll
Copy link
Owner Author

@zkf thanks - this seals the deal for me. Unless I back out this procedural clone feature, there's currently no way for immutable Patchinko to deal with objects like Sets or Maps without special-casing for them. Because there's a potentially infinite number of conceivable object types, native and author-land, where the desirable way of cloning them can't be inferred (should a DOM node in a data structure be cloned with node.cloneNode() or node.cloneNode(true)?), this effectively means with immutable Patchinko in its current state there is no way to perform deep modifications on special objects or even guarantee they can be queried, which is IMO an intolerable blocker.

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

No branches or pull requests

2 participants