barneycarroll/patchinko

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

barneycarroll opened this issue · 2 comments

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 commented

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} } }

@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.