Argument order for adopt()

[rbuckton]

This continues a discussion from #102 regarding the argument order of adopt(), the outcome of which is a condition for Stage 3 advancement.

As I understand it, it is @ljharb's position that the argument order should be adopt(onDispose [, value]), which would potentially make defer(onDispose) unnecessary due to overlap. The precedent for this would be the existence of the optional thisArg in existing APIs like Array.prototype.map, .filter, .forEach, and the optional initialValue argument to Array.prototype.reduce and .reduceRight.

It is the champion's position that adopt(value, onDispose) is the correct argument order, with the precedent being existing APIs such as JSON.parse, JSON.stringify, and Array.from, and that there are inherent differences in how .adopt is intended to be used vs. .defer that indicate these should be separate methods.

Rather than go into more detail for each argument in the issue description, I'll post specifics related to my rationale in the comments below.

@ljharb: If you have any further details you'd like to provide, please let me know in the comments as well. If I have misrepresented your position in any way, let me know and I can update the issue description to more accurately summarize your position.

[ljharb]

The precedent for "the optional thing being last" is also "virtually every modern userland API with optional arguments - even in node, where initially the convention was to always put the options bag last but to have optional arguments before it (that were optional even in the presence of the options bag), the ecosystem largely decided that was a brittle pattern and moved to the pattern we use in defaulting, where any argument that's omitted must also omit every argument that comes after it.

Can you elaborate on Array.from? JSON.parse and JSON.stringify are both very old APIs, and also being forced to write JSON.stringify(obj, null, 2) is widely regarded as a wart, so I don't think they're useful examples.

[rbuckton]

The precedent for "the optional thing being last" is also "virtually every modern userland API with optional arguments - even in node, where initially the convention was to always put the options bag last but to have optional arguments before it (that were optional even in the presence of the options bag), the ecosystem largely decided that was a brittle pattern and moved to the pattern we use in defaulting, where any argument that's omitted must also omit every argument that comes after it.

This argument presupposes that the value is optional, however I contend that the value is vital. To explain this, let me start with a summary of the use() method and using declarations in general:

The using declaration is intended to represent an RAII-style declaration (Resource Acquisition is Initialization). One of the key tenets of this approach is that acquisition of the resource is as close as possible to its initialization so that any necessary cleanup steps can be recorded without the possibility of user code intervening that could result in an exception that would prevent cleanup:

{
  using x = new Resource();
  ...
}

You want as little user code as possible between new Resource() and runtime recording its Symbol.dispose method in the environment. If an exception could occur in between these two operations, it would increase the possibility of a leak or race condition since the cleanup step would not execute.

The use() method of a DisposableStack is intended to be used in the same way:

{
  using stack = new DisposableStack();
  const x = stack.use(new Resource());
  ...
}

As little user-code as possible should run between resource acquisition (i.e., new Resource()) and initialization (i.e., const x = ). Here, the only exceptions that might be thrown are the same that you would see with a using declaration (i.e., the object doesn't have a callable Symbol.dispose method).

This is especially important if the return value of use() is passed as an argument:

f(stack.use(new Resource1()), stack.use(new Resource2()));

Here, we ensure resources are added to the stack in the order they are acquired, so that cleanup can occur in the correct order later.

The purpose of the adopt() method is to model the same behavior as using and the use() method, except with non-disposable values. Again, this means that there should be little to no user-code between resource acquisition and initialization:

{
  using stack = new DisposableStack();
  const x = stack.adopt(new Resource(), res => res.close());
  ...
}

The adopt() method also performs the same checks as using and use(), and in the same order:

1. The resource is acquired (i.e., new Resource()).
2. The disposal function is read (in this case, as an argument).
3. The disposal function is verified to be callable.
4. The resource and disposal function are recorded.
5. The resource is returned (or the binding initialized, in the case of using).

Changing the order of arguments for adopt() would break that ordering, and would not align with the intent to emulate RAII behavior as closely as possible.

While there are obvious overlaps, defer() is intended for a completely different use case than use() or adopt(). While both use() and adopt() are intended for resource acquisition, defer() is primarily intended for non-resource cleanup and is primarily a convenience mechanism. It does not take a resource as input, and it does not return a value.

Can you elaborate on Array.from? JSON.parse and JSON.stringify are both very old APIs [...]

JSON.parse and JSON.stringify are no older than Array.prototype.filter or many other APIs that accept an optional, trailing thisArg. Since the introduction of arrow functions to JS, the thisArg argument is used less and less frequently, to the point where it has been completely elided from the new methods in the Iterator helpers proposal, so a suggestion to emulate thisArg-like behavior is unconvincing.

In Array.from, the argument order is (items, mapfn), placing the subject (items) first and the callback second. In JSON.parse and JSON.stringify, again the subject comes first and the callback comes second. This is generally the same in userland libraries such as underscore, lodash, and JQuery, and in other platform APIs including the DOM, NodeJS, Electron, etc. It's also very frequently the case that a parameter that is intended to be returned, unchanged, from a function be the first parameter of that function.

[...] being forced to write JSON.stringify(obj, null, 2) is widely regarded as a wart, so I don't think they're useful examples.

This isn't an argument against the subject-first ordering, but an argument that the callback should potentially be further to the right of the subject.

[zloirock]

I'm strictly for the current behavior since .adopt(onDispose, value) is absolutely unintuitive.

[zloirock]

const foo = stack.use(new Foo());
const bar = stack.adopt(new Bar(), it => {
  somehowClean(it, 42);
});

vs

const foo = stack.use(new Foo());
const bar = stack.adopt(it => {
  somehowClean(it, 42);
}, new Bar());

In the second case, I don't immediately see what bar is.

[ljharb]

@rbuckton

Here, we ensure resources are added to the stack in the order they are acquired, so that cleanup can occur in the correct order later.

I'm 100% with you up to here.

The disposal function is verified to be callable.

So it sounds like the concern is that if the disposal function is NOT callable, there'd be an exception, before the value has been registered for disposal. That makes sense - except that all argument validation must (imo and by convention) be done before doing anything else with the values, which means that the argument order is irrelevant - a non-callable disposal function will always throw before the resource is registered for disposal if it's done in the same call.

JSON.parse and JSON.stringify are no older than Array.prototype.filter

Yes, they are; JSON functions were popularized in the ecosystem long before their inclusion in ES5, which is when filter was added.

I'm not sure where I suggested to use thisArg behavior in any way?

[rbuckton]

The disposal function is verified to be callable.

So it sounds like the concern is that if the disposal function is NOT callable, there'd be an exception, before the value has been registered for disposal. That makes sense - except that all argument validation must (imo and by convention) be done before doing anything else with the values, which means that the argument order is irrelevant - a non-callable disposal function will always throw before the resource is registered for disposal if it's done in the same call.

This is far from the only concern, and if that is your only takeaway then I fear you have missed my point. Both adopt() and use() model the same specific intention and behavior. In both of these methods it is the resource that is subject and primary focus of the operation. Resource allocation should be front and center, not buried at the tail end of an argument list after an arbitrarily long callback.

It is defer() that differs in intent, and is provided primarily as a convenience for callback-specific cleanup, where there is no resource to capture.

I'm not sure where I suggested to use thisArg behavior in any way?

You mentioned it multiple times in #102, including this comment.

Regardless, I'm still not convinced by your argument that the resource is optional. Yes, in theory you could reorder arguments to make adopt and defer a single method, but that would break with the intention behind the individual behaviors we are trying to model. It would also result in a poorer developer experience per both the inference argument I made in #102 (comment), and examples like the one @zloirock made above.

[ljharb]

I think the validation thing is more important to discuss.

Regardless of the argument order, the callback needs to be checked for callability before registering the resource for disposal.

[rbuckton]

I think the validation thing is more important to discuss.

Regardless of the argument order, the callback needs to be checked for callability before registering the resource for disposal.

That is what occurs in the current design as well, which is why I do not understand why you consider this to be more important.

[ljharb]

I'm just trying to interpret your long comment above.

Whether the argument order is resource, onDispose or onDispose, resource, the same amount of user code runs, and the same possibility for exceptions exist.

It sounds like you're saying that you care about having the resource appear first because of adherence to this RAII philosophy, and also because it's more readable to see the resource being disposed before reading the disposal callback?

To confirm, is stack.adopt(resource, onDispose) the same as (stack.use(resource), stack.defer(onDispose))?

[rbuckton]

I'm just trying to interpret your long comment above.

Whether the argument order is resource, onDispose or onDispose, resource, the same amount of user code runs, and the same possibility for exceptions exist.

It sounds like you're saying that you care about having the resource appear first because of adherence to this RAII philosophy, and also because it's more readable to see the resource being disposed before reading the disposal callback?

Yes

To confirm, is stack.adopt(resource, onDispose) the same as (stack.use(resource), stack.defer(onDispose))?

No, you cannot use use() with an object that does not already have a [Symbol.dispose] method, so your example would fail. The closest equivalent to something like const x = stack.adopt(new Resource(), onDispose), using only defer() would be:

let temp;
const x = (temp = new Resource(), stack.defer(() => onDispose(temp)), temp);

[rbuckton]

I feel you may have misunderstood the difference in the APIs. Perhaps adding type information might clarify:

class DisposableStack {
  use<T extends Disposable | null | undefined>(disposableResource: T): T;
  adopt<T>(nonDisposableResource: T, onDispose: (resource: T) => void): T;
  defer(onDisposed: () => void): void;
}

use() can only used with objects that have a [Symbol.dispose](), and is the primary mechanism for using a DisposableStack.

adopt() can be used with any value, and uses the provided callback as if it were the [Symbol.dispose]() method. Its intent is to model use(), but provide interoperability for non-disposable resources. The provided callback will be invoked with the resource as its argument.

defer() accepts a callback that is executed when the stack is disposed. Its callback takes no arguments.

{
  using stack = new DisposableStack();
  stack.use({ [Symbol.dispose]() { console.log(1); } });
  stack.adopt(2, resource => { console.log(resource); } });
  stack.defer(() => { console.log(3); });
  stack.use({ [Symbol.dispose]() { console.log(4); } });
}
// prints:
// 4
// 3
// 2
// 1

[ljharb]

Ron and I got on a call and we were able to get on the same page about the mental models and concepts, and I'm content with the proposal as-is, so we can consider this resolved, and the stage 3 condition met.