Problem with combine() and Mocks

[HyperCharlie]

I just spent the last few days chasing my tail on this one. I was using several mocking libraries, notably ts-mockito and testdouble, and got the same behavior. If I created a mock object, wrapped it with okAsync(), and tossed that into combine(), I would get nothing in the return- the mock object was being dropped. Here's an example test in jest that would fail:

import td from "testdouble";

interface ITestInterface {
    getName(): string;
    setName(name: string): void;
    getAsyncResult(): ResultAsync<ITestInterface, Error>;
}

describe("Debugging and basic info tests", () => {
test("combine works with TestDouble mocks of interfaces", async () => {
    // Arrange
    const mock = td.object<ITestInterface>();

    // Act
    const result = await combine([okAsync(mock)]);

    // Assert
    expect(result).toBeDefined();
    expect(result.isErr()).toBeFalsy();
    const unwrappedResult = result._unsafeUnwrap();
    expect(unwrappedResult.length).toBe(1);
    expect(unwrappedResult[0]).toBe(mock);
  });
});

If you run this, you'd find that the last two expect calls would fail, because combine would return an empty array! It would only do this with mocks, not with normal objects. So I dug into it, and I found a solution in the combine() code. You are using array.concat(nonArray) in the reduce() of combineResults(). I rolled my own combine and it works even with mocks. I added typing support for heterogenous lists as well, but it looks like you're working on a slicker solution in another issue. Here's my "fixed" combine:

export class ResultUtils {
    static combine<T, T2, T3, T4, E, E2, E3, E4>(asyncResultList: [ResultAsync<T, E>, ResultAsync<T2, E2>, ResultAsync<T3, E3>, ResultAsync<T4, E4>]): ResultAsync<[T, T2, T3, T4], E | E2 | E3 | E4>;
    static combine<T, T2, T3, E, E2, E3>(asyncResultList: [ResultAsync<T, E>, ResultAsync<T2, E2>, ResultAsync<T3, E3>]): ResultAsync<[T, T2, T3], E | E2 | E3>;
    static combine<T, T2, E, E2>(asyncResultList: [ResultAsync<T, E>, ResultAsync<T2, E2>]): ResultAsync<[T, T2], E | E2>;
    static combine<T, E>(asyncResultList: ResultAsync<T, E>[]): ResultAsync<T[],E> {
        return ResultAsync.fromPromise(Promise.all(asyncResultList), 
        (e) => {return e as E})
        .andThen(ResultUtils.combineResultList);
    }
    
    static combineResultList<T, E>(resultList: Result<T, E>[]): Result<T[], E> {
        return resultList.reduce((acc: Result<T[], E>, result) => {
            return acc.isOk()
                ? result.isErr()
                    ? err(result.error)
                    : acc.map((values) => {
                        values.push(result.value);
                        return values;
                    })
                : acc;
        }, ok([]));
    };
}

[supermacro]

Hey @HyperCharlie, sorry for the delay. I've just started a new job so I haven't had much time to groom issues here. I'll have a look this week.

[HyperCharlie]

Not a problem. Fortunately I have a workaround that's acceptable; I just wanted to let you know and hopefully save others the same headaches!

[supermacro]

Sorry to be a bother, but I was wondering if you might be able to link me to a codesandbox or some other service / repo with a working repro. That would help a lot!

[HyperCharlie]

Sure, I can set up something like that in the morning.

[supermacro]

Hey @HyperCharlie, any update on creating a small repro? Otherwise I will close this issue!

[HyperCharlie]

Hey Gio, I have not forgotten this, just been super busy. One of our engineers announced he was leaving and I've had to scramble to get projects he was on past the parts he was critical for.

…

On Wed, Feb 17, 2021 at 3:46 PM Gio ***@***.***> wrote: Hey @HyperCharlie <https://github.com/HyperCharlie>, any update on creating a small repro? Otherwise I will close this issue! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#228 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANC7E2UJ2AZGBT7YLXW5HELS7RIMHANCNFSM4WUNIXGA> .

[supermacro]

Thanks for the update! I'll leave it open.

[delitescere]

bump

[supermacro]

I spent some time this morning trying to figure out why this issue is occurring. @HyperCharlie provided a potential solution, but I first wanted to understand the cause of the problem.

I played around with the emitted output of neverthrow's v4.2.2 source code using this repro repository: https://github.com/supermacro/neverthrow-combine-issue-repro

With the above repo I then started playing with the neverthrow code inside of node_modules.

// inside of node_modules/neverthrow/dist/index.cjs.js

var combineResultList = function (resultList) {
    console.log(resultList[0].value) // <---- value is _NOT_ undefined here

    var resultList2 = resultList.map((result) => {
      console.log('> ' + result.value) // <----- value is undefined here
      return result
    })

    return resultList2.reduce(function (acc, result) {
        console.log('Val: ' + result.value) // <----- value is undefined here
          
        return acc.isOk()
            ? result.isErr()
                ? err(result.error)
                : acc.map(appendValueToEndOfList(result.value))
            : acc;
    }, ok([]));
};

Notice a few things:

• The mocked object is defined prior to iterating on the resultList
• It seems like iteration methods are doing something to the mocked object, or the mocked object is reacting to the fact that it's being iterated on
  • .map and .reduce turns the mocked object into undefined for some reason
  • This means that the mocked object is "dropped" prior to even calling Array.prototype.concat, so this doesn't seem to be the issue as was originally mentioned by @HyperCharlie

As it stands, I don't yet know the cause of the issue. Therefore I do not know what a good solution is at this moment.

Any thoughts @delitescere?

[delitescere]

const neverthrow = require('neverthrow');
var results = [neverthrow.ok('a'), neverthrow.ok('b')];
console.log(JSON.stringify(results));
var combined = neverthrow.combine(results);
console.log(JSON.stringify(combined));

assert(results[0].value === combined.value[0]);

The functors are results and combined.value. So, results[0].value === combined.value[0] but there is no combined[0] and therefore no combined[0].value.

[supermacro]

I've gone ahead and made an interactive sample of the above code on repl.it:

https://replit.com/@gDelgado14/WhichShortServerapplication#index.js

---

That said, can you elaborate on the point you are trying to make @delitescere? Not sure how this relates to the fact that iterating methods such as .map and .reduce are dropping the mocked value inside of a result.

[supermacro]

$75 USD bounty for anyone that can solve this

[supermacro]

Bounty increased to $150 USD

[kieran-osgood]

TLDR

This will be a bit lengthy, the first snippet is about the undefined logging in the maps, but the final two snippets are the ones where changes were made that matter for the original issue. The rest was just exploratory stuff incase it helps prompt more thoughts if I'm missing something important

---

I just wanted to play around and I seem to have gotten a case that works for what was shown above but I hope someone might be able to help verify this.

Undefined logging of snippets provided above

So repo you posted above (not the repl.it) I think the issue with the lines you've mentioned logging undefined is because you're console logging the result of string + result value (which in the case of const mock = td.object<ITestInterface>() will be an object) e.g. if you were to adjust your code to this:

 // inside of node_modules/neverthrow/dist/index.cjs.js
 var combineResultList = function (resultList) {
    console.log(resultList[0].value) // <---- value is _NOT_ undefined here

    var resultList2 = resultList.map((result) => {

     // Note using the comma, not the +
      console.log('> ', result.value) // <----- value is undefined here no longer!
      return result
    })
   
    // ... other stuff
};

Theres no undefined logs coming out, infact logging out the 'mock' variable will show just an output of { toString: [Function (anonymous)] }, going further and logging mock.getName does give [Function: testDouble] { toString: [Function (anonymous)] } - I'm not familiar on proxies but thats supposedly the way they're handling intercepting these calls to these objects and replacing them with whatever mocks are provided, which was throwing me off as being related, see below

Something weird I did notice is that the mock value seems to change after the combine?

describe("Debugging and basic info tests", () => {
  it("combine works with TestDouble mocks of interfaces", async () => {
    // Arrange
    const mock = td.object<ITestInterface>()
    console.log('check mock 1', mock);

    // Act
    const result = await combine([
      okAsync(mock),
      okAsync({ name: 'giorgio' }),
      okAsync(123),
    ] as const)

    console.log('check mock 2: ', mock); // second mock now is different 

    // Assert
    expect(result).toBeDefined()
    expect(result.isErr()).toBeFalsy()
    const unwrappedResult = result._unsafeUnwrap()

    expect(unwrappedResult.length).toBe(2)
    expect(unwrappedResult[0]).toBe(mock)
  })
})

the "check mock 2" log now has a value of:

 console.log
    check mock 2:  {
      toString: [Function (anonymous)],
      length: [Function: testDouble] {
        toString: [Function (anonymous)],
        [Symbol(Symbol.toPrimitive)]: [Function: testDouble] { toString: [Function (anonymous)] }
      },
      [Symbol(Symbol.isConcatSpreadable)]: [Function: testDouble] { toString: [Function (anonymous)] }
    }

Possible Solution

(it gets the tests passing, but I don't have full explanation as to why and am looking more)

As the author originally pointed out that its related to the concat method, im assuming its related back to the proxy implementation of this, but overall (and im not sure if theres any impacts to these changes so take a look and see if it prompts anything in anyone who knows this better than me, because its weird that this "worked"?) but it seems like it was trying to unfurl the objects as well (like it would with an array) so:

var appendValueToEndOfList = function (value) { return function (list) {
    // need to wrap `value` inside of an array in order to prevent
    // Array.prototype.concat from destructuring the contents of `value`
    // into `list`.
    //
    // Otherwise you will receive [ 'hi', 1, 2, 3 ]
    // when you actually expected a tuple containing [ 'hi', [ 1, 2, 3 ] ]
    if (Array.isArray(value) || typeof value === 'object' && value !== null) {
        return list.concat([value]);
    }

    return list.concat(value);
}; };

resulted in all of these assertions working

    expect(unwrappedResult.length).toBe(3)
    expect(unwrappedResult[0]).toBe(mock)
    expect(unwrappedResult[1]).toStrictEqual({name: 'giorgio'})
    expect(unwrappedResult[2]).toEqual(123)

[supermacro]

Hey @kieran-osgood, sorry for the delay in responding.

I'll be sinking my teeth into this issue and your response once again this week.

[supermacro]

I took some time to try and understand why this proposed solution works, and I'm not yet sure why it is the case.

I had a hypothesis that Proxy objects vanished by virtue of being concated onto an array, thus I wrote the following test in my repro repository:

https://github.com/supermacro/neverthrow-combine-issue-repro/blob/main/hello.test.ts#L28

And interestingly, the test passed... Thus, I think that something specific to the testdouble library is changing the default behaviour of proxies such that they disappear / become undefined when passed into an array.

[kieran-osgood]

@supermacro Yeah I definitely hesitated to call what I shared a proposed solution, really just initial findings from having played with it 😅 but I do think I've narrowed down the problem further!

I agree with what you're saying regarding it being related to the way they handle proxying the objects as that was what I was also thinking, and there was tvcutsem/harmony-reflect#19 which indicated that internally Array.prototype.concat checks for [[Class] = Array] and so proxies could fail - but as your test showed it only seems to be the way testdouble is doing it and I also couldnt replicate a proxy failing outside of td.object<>, so maybe a simple solution will be to move away from array.concat to avoid patching the prototype method?
e.g.:

 // inside of node_modules/neverthrow/dist/index.cjs.js
var appendValueToEndOfList = function (value) { return function (list) {
    const newList = list.slice()
    newList.push(value)
    return newList
}; };

Seems to get all tests running fine and removes the need to wrap arrays

This would also explain why my initial solution worked for the proxy issue, as the condition will allow the proxy through (and really any other object), and due to wrapping the value with [] the concat would detect its an array and which when unwrapped meant that the objects were passed through just fine, as well as the symbols. Essentially it just meant primitives went to the final concat, but given this I feel like really both solutions actually make more sense now

[supermacro]

This is great!

If you're willing to submit a fix PR with some tests (perhaps installing testdouble to write a related test) I'd be more than happy to send over the bounty your way 🙂

[kieran-osgood]

Awesome! I'll get a PR and tests set up either tonight or tomorrow and pop it over 😊

[supermacro]

Merged. Contacting you to send over bounty!

[kieran-osgood]

@supermacro That's great! How do you usually do this, I can give you my twitter to speak easier? https://twitter.com/kieranbosgood