Reintroduce asserting types when values are uncertain

[simontong]

I'm not sure if this belongs under bugs or a feature, but in version 1, it was possible to use jest-extended matches in toMatchObject to match by type like so:

type Shape = typeof data;

const data = {
  str: "abc",
  bool: true,
  num: 123,
  obj: {},
};

expect(data).toMatchObject<Shape>({
  str: expect.toBeString() as string,
  bool: expect.toBeString() as boolean,
  num: expect.toBeString() as number,
  obj: expect.toBeString() as object,
});

Since the introduction of the Result type, this is now throwing a warning in Typescript:

TS2352: Conversion of type 'Result' to type 'string' may be a mistake because neither 
type sufficiently overlaps with the other. 

Is it possible to reintroduce this functionality with the addition of generics for asserting the type passed in (removing the need to assert with as string / boolean / number),
i.e:

expect(data).toMatchObject<Shape>({
  str: expect.toBeString<string>(),
  bool: expect.toBeString<boolean>(),
  num: expect.toBeString<number>(),
  obj: expect.toBeString<object>(),
});

[simontong]

After investigating other options it looks like I can also produce the desired result with:

type Shape = typeof data;

const data = {
  str: "abc",
  bool: true,
  num: 123,
  obj: {},
};

expect(data).toMatchObject<Shape>({
  str: expect.any(String) as string,
  bool: expect.any(Boolean) as boolean,
  num: expect.any(Number) as number,
  obj: expect.any(Object) as object,
});

Which is an improvement in the right direction. Would it be possible to detect the correct type from the passed in Shape type?

[keeganwitt]

This is an unusual way to use toMatchObject. That function is meant to take an object as its argument. See https://jestjs.io/docs/expect#tomatchobjectobject.

I'd think a better test would be

type Shape = {
    str: String,
    bool: boolean,
    num: Number,
    obj: Object,
};
const data = {
    str: "abc",
    bool: true,
    num: 123,
    obj: {},
};
expect(obj instanceof Shape).toBeTrue();

If I understood the use case better, maybe I could suggest a better option.

I made the change to add the Result type. What's returned by toBeString() matches the Result type. So I think this was still an improvement.

[simontong]

The previous usage was very versatile because you could match some of the properties on an object with their respective values, and where you didn't know the value you could pass the expect.toBeType(), thirdly you could pass a expect.stringMatching() in as well, super useful!

expect(data).toMatchObject({
  cat: "dog",
  hello: expect.stringMatching(/, world$/),
  foo: true,
  colors: expect.toBeArray(),
  age: expect.toBeNumber(),
});

[keeganwitt]

The only reason such a usage worked before was because it was using an any before, bypassing type safety checks. The only thing I can suggest is to cast to any before passing to Jest's toMatchObject(). I can't think of any change that'd make sense in jest or jest-extended for this, but am open to suggestions if you can think of any.

Generifying functions like toBeString doesn't make sense because what string would be returned? We always pass the pass/fail boolean and the message as the result for all matchers. Passing anything else would be weird, possibly breaking, and only existing to bypass type safety.

[keeganwitt]

I'll discuss with the other maintainers about the return type we have now compared to @types/jest, but the explicit type seemed like an improvement.

[SLKnutson]

The explicit types are technically correct, but as far as I am aware, not useful for the consumers of jest-extended. There may be use cases that I'm not familiar with.

You don't need to actually return a string to say that the type returned is a string. It's sloppy but extremely convenient, and would probably likely result in fewer bugs for end users because casting to any/unknown totally destroys any type information.

Thanks!

[keeganwitt]

It might be useful when creating a new matcher that uses other matchers in its implementation or when unit testing said new matcher. Or just to have autocompletion, should you have any reason to look at the result.

What we're talking about is really whether jest-extended itself should lose the type information or whether the user's project using it should. I'm not really seeing a good reason to do the former. @SimenB any thoughts here?

[keeganwitt]

Actually, I was looking at the runtime return type

// test Jest matcher
const result1 = expect('2').toBeDefined();
console.log(result1);  // prints undefined

// test jest-extended matcher
const result2 = expect('2').toBeString();
console.log(result2);  // prints undefined

And both were undefined. I think I was confused between the return type of the matcher and the return type of the expect call.

I'm now thinking I should change this. Jest's Expect interface returns any (I think I misread it before).

[SimenB]

Note that DT is not the source of truth - Jest itself should be. And in Jest they don't return any 🙂

https://github.com/facebook/jest/blob/79977c4a296102ec0103271966dc305e8da82e1a/packages/expect/src/types.ts#L87-L119

---

any of course isn't wrong (it cannot be, by definition), but it's inaccurate. Though it probably doesn't matter in this context.

[keeganwitt]

I thought about returning void. That'd be more correct. It looked like the only reason DT didn't was because of a linting problem. Putting it back the way it was is at least better than the change I made. I'll open a separate PR to consider void.

[aldex32]

Hi @keeganwitt , are you planning to open a PR as you mentioned in your last comment?

[aukevanleeuwen]

Just adding some thoughts to this thread as well: the change to any is a bit unfortunate for us. We use this pattern quite a lot:

expect(something).toEqual({
    // ...
    createdAt: expect.toBeDateString() as string,
    expiryTime: expect.toBeDateString() as string,
});

Since this PR is merged we need to add the as string to keep our linter happy. Otherwise we would violate this rule:

109:25  error  Unsafe assignment of an `any` value  @typescript-eslint/no-unsafe-assignment

We don't really want to disable that rule (even in test-code), so I would also be in favour returning at least something not any. Maybe unknown, maybe void? I don't really know what it actually returns in terms on vanilla JS.

[keeganwitt]

Hi @keeganwitt , are you planning to open a PR as you mentioned in your last comment?

Thank you for the reminder. PR here: #535

[keeganwitt]

Just adding some thoughts to this thread as well: the change to any is a bit unfortunate for us. We use this pattern quite a lot:

expect(something).toEqual({
    // ...
    createdAt: expect.toBeDateString() as string,
    expiryTime: expect.toBeDateString() as string,
});

Since this PR is merged we need to add the as string to keep our linter happy. Otherwise we would violate this rule:

109:25  error  Unsafe assignment of an `any` value  @typescript-eslint/no-unsafe-assignment

We don't really want to disable that rule (even in test-code), so I would also be in favour returning at least something not any. Maybe unknown, maybe void? I don't really know what it actually returns in terms on vanilla JS.

I'm thinking the void change being considered would not work for your use case (even though it reflects reality). Could you drop in a jest-extended.d.ts file into your project and see how making the Expect interface return void works for you?

[aukevanleeuwen]

@keeganwitt

Right, this issue got a bit off my radar, but since updating to the version that included #535, it popped up again :-)

For us - for most cases - void works reasonably well. It's not any so we don't have any linting warnings (👍) on that. We can use it in simple cases like:

const actual = {
    date: new Date().toString(),
};

expect(actual).toEqual({
    date: expect.toBeDateString(),
});

There are a few cases where the expected object is also typed, most notably in mocked toHaveBeenCalledWith kind of expectations¹:

interface SomeType {
    date: string;
}

const mocked = jest.fn<Promise<object>, [SomeType]>();

expect(mocked).toHaveBeenCalledWith<[SomeType]>({
    date: expect.toBeDateString(),
});

This fails on TS2322: Type 'void' is not assignable to type 'string'.. I doubt we can really type this correctly though. The only thing is, since now it's always void, we can ignore this error with an explicit cast, but I first have to cast it to unknown because if I do as string I end up with TS2352: Conversion of type 'void' to type 'string' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.. This is where void is a bit annoying.

As a response to you saying 'the runtime type is void': this seems a bit simplistic. Jest does quite some magic behind the scenes and how you are using it gives different results. Compare:

describe('foo', () => {
    it('should foo', () => {
        const actual = {
            date: new Date().toString(),
        };
        const expected = {
            date: expect.toBeDateString(),
        };

        console.log('expected', expected);

        expect(actual).toEqual(expected);
    });

    it('should bar', () => {
        const actual = new Date().toString();

        const result = expect(actual).toBeDateString();

        console.log('result', result);
    });
});

  console.log
    expected {
      foo: 'foo',
      date: CustomMatcher {
        '$$typeof': Symbol(jest.asymmetricMatcher),
        sample: [],
        inverse: false
      }
    }

      at Object.<anonymous> (__tests__/utils.test.ts:16:17)

  console.log
    result undefined

      at Object.<anonymous> (__tests__/utils.test.ts:26:17)

So at least it should be void | <something here (CustomMatcher?)>. This still wouldn't solve the case above with the toHaveBeenCalledWith(..) because it would still complain about the 'does not sufficiently overlap with string. So maybe unknown would have been more convenient.

I'm not entirely sure if I'm really asking for a change though. This is better than any (for us), but I'm saying that having it be void is also not entirely correct...

---

¹ I understand that this feels a bit convoluted (to explicitly type the argument of the mock). If we just let it be inferred, it's all fine. However there is a library that we are using (https://github.com/m-radzikowski/aws-sdk-client-mock) that provides these kind of typed mocks, so this is where we are running into it.

[keeganwitt]

I think expect('').toBeString() returns the result from a matcher, but expect.toBeDateString() returns the matcher itself. When I talked about "the runtime return type" I thought it was what Jest itself is doing, and what I observed being returned

test('check expect returns undefined', () => {
    const toBeStringResult = expect('').toBeString();
    expect(toBeStringResult).toBeUndefined();

    const toBeDateStringResult = expect('2020-01-01').toBeDateString();
    expect(toBeDateStringResult).toBeUndefined();
});

Your use case is creating an expected object with a matcher in it, which is a different usage. I'll study how toEqual() works in such cases.