Improve sandbox manager API

[sisp]

This PR contains an (in my opinion) improvement of the sandbox API.

The sandbox manager's withSandbox method now requires an array of nodes from the original tree and the corresponding sandbox nodes are now positional arguments of the callback function. E.g.:

// before
sandboxManager.withSandbox(a, (sa) => { ... })
// after
sandboxManager.withSandbox([a], (sa) => { ... })

// before
sandboxManager.withSandbox([a, b], ([sa, sb]) => { ... })
// after
sandboxManager.withSandbox([a, b], (sa, sb) => { ... })

Note that this PR contains a breaking change in the signature of the withSandbox method.

@xaviergonz What do you think? I hope a minor breaking change in this isolated feature is okay.

[xaviergonz]

I don't see anything wrong with the change (the sandbox was your idea after all). I'm curios though, why do you feel the new syntax is better?

[sisp]

I think the new API is more consistent and intuitive because nodes are always passed to sandbox.withSandbox as an array of nodes and the callback function has a corresponding number of arguments. In case of a single node, the array has one element and the callback function has one argument. Before, a single node could be passed in two ways: (1) wrapped by an array and (2) without an array. This was ambiguous, I just thought the new way is nicer. 🙂

[xaviergonz]

Did you consider this alternative?

sandboxManager.withSandbox(a, (sa) => { ... })
sandboxManager.withSandbox(a, b, (sa, sb) => { ... })

I think with newer versions of typescript it should be possible to type that correctly

[sisp]

No, I hadn't thought about that. Do you think it's better?

[xaviergonz]

If the main use case is a single node then I'd think so

[sisp]

I've been using multiple nodes quite often actually. But it probably depends on the use case.

[xaviergonz]

Then up to whichever you think the main use case is :)

[sisp]

I think your suggestion may have an advantage: It is clearer that the array is not meant to be a tweaked array but just a collection of nodes. I'll think about it until tomorrow and probably try to get the types working for your suggestion, at least to see whether it's possible.

[sisp]

This is the best I could come up with (using variadic tuple types and labeled tuple elements introduced in Typescript 4.0):

   withSandbox<T extends readonly [object, ...object[]], R = void>(
-    nodes: T,
-    fn: WithSandboxCallback<T, R>
+    ...args: [...nodes: T, fn: WithSandboxCallback<T, R>]
   ): R {
+    const nodes = (args.slice(0, -1) as unknown) as T
+    const fn = (args[args.length - 1] as unknown) as WithSandboxCallback<T, R>
     for (let i = 0; i < nodes.length; i++) {
       assertTweakedObject(nodes[i], `nodes[${i}]`)
     }

Unfortunately, there seems to be no way of destructuring a tuple where the rest argument is at the beginning:

const [...nodes, fn] = args // ERROR: A rest element must be last in a destructuring pattern. ts(2462)

And I couldn't find a way to split nodes and fn in a way that correctly preserves the types. The type casts are a bit ugly, but they are not visible to the user, so it may be okay to do it this way.

Or can you think of a better solution?

[xaviergonz]

Until TS 4.2 arrives (microsoft/TypeScript#41544) I can only think of overloads:

// overloads
function f<N0 extends object>(node0: N0, callback: (node0: N0) => void): void
function f<N0 extends object, N1 extends object>(node0: N0, node1: N1, callback: (node0: N0, node1: N1) => void): void
// and so on until 10 args or so

// base case, type not exported
function f(...args: any[]) {
  const nodes = args.slice(0, args.length - 1);
  const callback = args[args.length - 1];
}

f({x: 10}, (n) => {})
f({x: 10}, {y: 20}, (n, n2) => {})

However overloads won't work for destructuring arrays though.

so which one would you prefer, overloads, then proper support once TS4.2 arrives (I think it is due to arrive in a few days) or just use arrays?

[sisp]

I think I prefer your suggested API. I also noticed it is similar to, e.g., reselect's createSelector signature. But I suggest to wait until Typescript 4.2 has been released instead of temporarily using overloads.

Edit: Oh, well, createSelector also supports an array. 😄

[sisp]

Okay, since it seems there is no clear winner, let's go with the array (current state of this PR) because it has better type support. Even with Typescript 4.2, type assertions are needed which is still a little ugly in my opinion.

[xaviergonz]

Merged then, thanks! :D