investigate(testing): explicit resource management for `spy()` and `stub()`

[iuioiua]

Currently, one must execute .restore() on a spied (?) or stubbed object to restore the original function/instance to its initial behaviour. It'd be great if spy() and stub() returned instances implementing the [Symbol.dispose] method that would automatically perform this restoring method once out of scope by using the using keyword. This might involve implementing new underlying classes and reworking the functions, moving them away from using Object.defineProperties.

The same goes for any other applicable APIs in the Standard Library.

Inspired by #3961

Further reading:

• https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html#using-declarations-and-explicit-resource-management
• https://deno.com/blog/v1.38#using-with-deno-apis

[syhol]

SWC make's this hard to do without breaking current functionality.

The spy in its current form needs to be a function, so you can use it like this:

import { spy } from "https://deno.land/std/testing/mock.ts";
const mySpy = spy(Date, "now");
mySpy();

SWC's using seems to have this check:

if (typeof value !== "object") {
  throw new TypeError("using declarations can only be used with objects, null, or undefined.");
}

I don't know any way pass the typeof value !== "object" check and also support calling the returned spy.

Would you be open to a breaking change and having method spies being objects instead of functions?

Someone could ask SWC if this is something they could change. If not then I think it's something the deno typechecker should pick up on, because I got a runtime error rather than a static one.

[iuioiua]

spy() can still be a function and return a disposable class instance from under the hood, no? I'm not sure I'm following.

[syhol]

I'm just talking about the return value of spy(), thats mySpy in the example above. spy itself will always be a function.

You could make that return value be a disposable class instance, but then it couldn't also be callable like a function, as it is in the example above.

It's important that the return value of spy is a function for the case of the FunctionSpy, when you would wrap a plain function rather than a object method. For example:

// FunctionSpy
const decodeSpy = spy(decodeURI);
// This is the only way to call the spy
decodeSpy();

whereas when you spy on a method (aka MethodSpy)

// MethodSpy
const randomSpy = spy(Math, "random");
// This is one way to call the spy
randomSpy();
// This is another way to call the spy, as the property is overridden
Math.random();

One option is to keep the FunctionSpy as a function, but change the MethodSpy to instead return an object with dispose property.

// FunctionSpy
const decodeSpy = spy(decodeURI);
// This next line continues to work as before
decodeSpy();

// MethodSpy (can now use "using")
using randomSpy = spy(Math, "random");
// This next line is no longer valid as it wouldn't be a function, would fail typecheck and throw error.
randomSpy();
// This next line continues to work as before
Math.random();

[KyleJune]

SWC's using seems to have this check:

if (typeof value !== "object") {
  throw new TypeError("using declarations can only be used with objects, null, or undefined.");
}

I don't know any way pass the typeof value !== "object" check and also support calling the returned spy.

Would you be open to a breaking change and having method spies being objects instead of functions?

Someone could ask SWC if this is something they could change. If not then I think it's something the deno typechecker should pick up on, because I got a runtime error rather than a static one.

Does TypeScript have that same restriction to the using keyword or is this unique to SWC? If it's unique to SWC, it seems like changing it to also allow functions would be a pretty easy change by updating that if statement.

It seems like that object check is the only thing holding back the cleanest approach that wouldn't be a breaking change to the spy/stub functions. If we can't just simply add the [Symbol.dispose] property to our spy/stub functions and have the Spy/Stub interfaces implement Disposable. I think there is a minimal breaking change we could make to support using spies and stubs on instance methods.

Normally, the only cases where you need to restore are when you are spying or stubbing on method properties. When doing that, users wouldn't typically call the spy or stub that was created directly, it would be called by calling the method on the class. We could do a breaking change of having spy and stub return a class instead of a function when spying on or stubbing instance methods, but still return a function if spying on a function. We could do that by renaming the Spy interface to SpyFunction and add a new Spy class that is disposable.

// non disposable spy functions
const cbSpy = spy(); // returns a SpyFunction like it currently does.
const cbSpy = spy(() => { /* ... */ }); // returns a SpyFunction like it currently does.
// disposable spies and stubs
using randomSpy = spy(Math, "random"); // returns a Spy class that implements disposable.
using randomStub = stub(Math, "random", () => 42); // returns a Stub class that implements disposable.

So while it would be a breaking change, it probably wouldn't impact many if any users. In that above example, cbSpy would still be callable, it just wouldn't be disposable. The randomSpy and randomStub would not be callable but would be disposable. If someone were calling randomSpy or randomStub, they could easily just switch to calling Math.random() directly.

[babiabeo]

This is an example of using using keyword with function:

interface Calculation extends Disposable {
    (x: number, y: number): number;
}

const impl: Calculation = Object.assign(
    (x: number, y: number) => x * y,
    {
        [Symbol.dispose]: () => {
            console.log("Hello World");
        },
    },
);

function calculation(): Calculation {
    return impl;
}

using mul = calculation();
console.log(mul(4, 5));

And this is the code after transforming from TypeScript to JavaScript using tsc:

"use strict";
var __addDisposableResource = (this && this.__addDisposableResource) || function (env, value, async) {
    if (value !== null && value !== void 0) {
        if (typeof value !== "object" && typeof value !== "function") throw new TypeError("Object expected.");
        var dispose;
        if (async) {
            if (!Symbol.asyncDispose) throw new TypeError("Symbol.asyncDispose is not defined.");
            dispose = value[Symbol.asyncDispose];
        }
        if (dispose === void 0) {
            if (!Symbol.dispose) throw new TypeError("Symbol.dispose is not defined.");
            dispose = value[Symbol.dispose];
        }
        if (typeof dispose !== "function") throw new TypeError("Object not disposable.");
        env.stack.push({ value: value, dispose: dispose, async: async });
    }
    else if (async) {
        env.stack.push({ async: true });
    }
    return value;
};

// and more...

We can see that there is a line in __addDisposableResource:

if (typeof value !== "object" && typeof value !== "function") throw new TypeError("Object expected.");

It means that the using keyword can be used with objects and functions in TypeScript.

I don't know why SWC doesn't allow functions while TypeScript does. Could it be a mistake or is there some problem that prevents them from allowing the "using" keyword to be used with functions?

[KyleJune]

It means that the using keyword can be used with objects and functions in TypeScript.

I don't know why SWC doesn't allow functions while TypeScript does. Could it be a mistake or is there some problem that prevents them from allowing the "using" keyword to be used with functions?

Based on this, I don't think any breaking changes should be made to this API to support using. The Spy and Stub interfaces should be updated to extend Disposable and the dispose symbol should be added onto the functions returned by spy and stub.

But it will just have to wait on the SWC issue being fixed. It looks like it would be an easy change to make but I'm not too familiar with SWC and haven't contributed to it before.

[KyleJune]

The issue has been fixed in swc v1.4.0. Once Deno upgrades to that version, this feature could be implemented in a backwards compatible way.

[iuioiua]

Great work, @KyleJune! I've opened https://github.com/denoland/deno_ast/issues/210 to get [email protected] into the runtime.

[KyleJune]

I've put up a PR for implementing this. It just needs to wait on https://github.com/denoland/deno_ast/issues/210.