Typescript types adding unwanted / buggy 'Chai.Assertion' type requirement on should function interface return

[ActuallyHappening]

Describe the bug

When this package is installed in a fresh vite project, a function that has a type annotation to return an interface (of anything, as long as it is not an empty interface) mysteriously requires that a returned object's should key be of type Assertion if it returns an object literal

This is a problem, even if it is intended behaviour because a larger project that contains a function anywhere (in typescript) generally suffers from this mysterious behaviour.
It was not immediately obvious that this typing behaviour was from this package, however it is clearly the source or is causing it, an issue with this repository.

This bug was initially found by refactoring large sections of typing after installing this library.
This library extends the Object interface to add should: Chai.Assertion, which was very annoying to identify and results in this package being uninstalled (as it was not being used at the time)

Reproduction

Exact file from repo,
git clone https://github.com/ActuallyHappening/messing-about.git

Code (from repo):

interface test {
  anything: "here";
  asLongAs: "there is at least one property";
  inThe: "interface";
}
function func(): test {
  return {
    should: "what is this `Assertion` type?", // Error here, copy-pasted below
    /*
    Type 'string' is not assignable to type 'Assertion'.ts(2322)
    index.d.ts(1940, 5): The expected type comes from property 'should' which is declared here on type 'test'
    */
  };
};

System Info

System:
    OS: macOS 12.6
    CPU: (8) arm64 Apple M2
    Memory: 108.75 MB / 8.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.0/bin/npm
  Browsers:
    Brave Browser: 104.1.42.97
    Chrome: 105.0.5195.125
    Safari: 16.0

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[sheremet-va]

Technically this is expected behaviour, because chai can add should property on any object, but I also didn't know it existed 🤔

Don't know what to do with this 🤷🏻

[ActuallyHappening]

It was an interesting experience to debug, I had never heard of Chai.Assertion yet my functions seemed to want to return this type,
took a while to narrow down to this package (large project)

That is why I raised it as an issue as it was such an obscure bug (try searching it on google!)

[ToshB]

I have a similar problem, the interface pollution by chai breaks compilation when I'm also using types from @elastic/elasticsearch, which also has an object with a should-property. Identical to this issue.

It's been over a year since the chai-types module owners were pinged in the github discussion on this, without any reaction.

I suggest finding a way to drop the @types/chai dependency from vitest, so I can choose for myself whether to include it or not.

[roydukkey]

Additionally, I think it would be beneficial to have the two api's be (optionally) independent of one another.

For example, I might want to limit my repo to only using the Jest API, therefore I wouldn't even want to be bother seeing the Chai API in my autocompletes.

This would be a big improvement in helping young developers contribute tests, since they wouldn't have to muddle through questions like:

Are these two the same? Which one is better? Which one is expected for this repo?

expect(1).to.equal(1);
expect(1).toBe(1);

[dallas-flywheel]

This is the main issue preventing our team from adopting vitest. I also had a difficult time finding the error, and I'm very glad this issue exists.

We have an absurd amount of OpenSearch Query objects in our code-base that look something like this:

const queryBool = {
  bool: {
    should: [
      {
        term: { foo: "bar" }
      }
    ]
  }
}

Even something like this errors:

const query: OpenSearchBool["bool"] = {};
// Types of property 'should' are incompatible.

And, of course, doing something like this is invalid:

interface Object {
  should?: any;
  // All declarations of 'should' must have identical modifiers.
}

We could start doing "bad things" to try and work around this issue, like adding a post-install script in our package.json like this:

{
  "scripts": {
    "post-install": "rm node_modules/@types/chai/index.d.ts",
  }
}

But I feel like that is going down a dark path.

Typescript does not currently have a way to exclude the types included by packages or isolate global interface pollutions like this, but the feature request has been hotly debated.

This is especially an issue in the nx mono-repo world, where vitest is installed in the central node_modules, and from there, the @types/chai library pollutes the Object interface for every project within the repo.

This is an extra-tough issue, since the offending types are in your dependency, and they likely will not be changing. You do, however have control over which dependencies you include in the main vitest package. I second the recommendation to remove the @types/chai dependency from vitest and only allow it back in from an explicit install of a separate package. Or to create/use a fork that does not pollute the Object interface.

Until that happens, we'll have to go back to another testing environment, which (given the enthusiasm from the team) is disappointing.

[ActuallyHappening]

Glad this issue helps. Currently working with wonderful, pure Rust types. I don't miss typescript at all.

[sheremet-va]

@keithamus I am not sure who is responsible for @types/chai, but do you know if this issue can be fixed somehow? All linked chai issues don't have any discussions there.

[keithamus]

FWIW this is the first I've heard of this.

@types/chai is maintained within DefinitelyTyped. I'm not sure any chai maintainers have had much input there. I don't know how fixable it is.

should is only loaded in Chai when you call .should(). Chai also includes the modules chai/register-expect/chai/register-assert/chai/register-should and the chai/register-should import automatically does that for you. The DefinitelyTyped types simply apply it to Object though, even if you didn't import chai/register-should or call the should() function: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/98e68ce4add1e1fc4aad17402ae8eaab9ca15088/types/chai/v2/index.d.ts#L305-L307.

interface Object {
    should: Chai.Assertion;
}

Perhaps the types need to change such that the should() call makes that happen, off the top of my head something like:

function should(): Object is Object & {should: Chai.Assertion} {}

But I don't know if that's valid TS, or if it is, whether it is actually usable for users of the should syntax.