Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Promise #1570

Open
BryanHuntNV opened this issue Jan 5, 2024 · 15 comments
Open

Add support for Promise #1570

BryanHuntNV opened this issue Jan 5, 2024 · 15 comments

Comments

@BryanHuntNV
Copy link

It appears that chai-as-promised does not work with chai v5 and is no longer under development. Given the prevalence of programming with Promise, would it make sense to include the functionality of chai-as-promised into chai proper?

@43081j
Copy link
Contributor

43081j commented Jan 5, 2024

that is what i'm leaning towards

think it makes sense to have native support for things like toThrow on a promise, etc

@keithamus
Copy link
Member

An innate problem with adding promises to core is that all existing assertions synchronously throw, and there is no very clear signal as to what would then turn that into an asynchronous throw. chai-as-promised adds the eventually chainable property, which we could add but the await keyword is something that developers are much more likely to be aware of.

One way we could go about this change is to make all assertions return a Promise, but that's a big breaking change (though arguably one that gives us future flexibility for all kinds of async stuff). Another solution could be to extensively document how one would unpack promises within the existing assertions (which typically looks like expect(await ...), but the far bigger problem today exists with how to properly handle rejections which involves some code gymnastics.

@BryanHuntNV
Copy link
Author

FWIW, my typical use-case for chai-as-promised (using mocha) is:

return expect(testClass.testFunction(args)).to.eventually.be.rejectedWith(Error);

@43081j
Copy link
Contributor

43081j commented Jan 10, 2024

i recall in some other test libraries, some smarts were done to change the return type

e.g.

expect(someSyncThing).to.equal(10);

await expect(someAsyncThing).to.equal(10);

and basically have a conditional return:

function equal(expectation: unknown): T extends PromiseLike<unknown> ? Promise<void> : void;

but that may make my fancy types im doing in the typescript branch a bit more difficult 😬

since i'm not sure we can do a type guard return of a promise

@perrin4869
Copy link

perrin4869 commented May 2, 2024

I would also vote in favor of adding the eventually, become, rejectedWith, etc APIs found in chai-as-promised, I've used those since forever ago, and it's the one thing preventing me from upgrading to v5

@mscharley
Copy link

As much as I'm all for getting these into chai itself, chai-as-promised works perfectly fine for me at least with chai v5.

@mscharley
Copy link

Hmm, now I look at the original issue, I can't remember why I ended up subscribed to this ticket. I just updated a project to chai v5 which was using chai-as-promised and it seems to be working fine.

// mocha.env.js
const { use } = await import("chai");
use((await import("chai-as-promised")).default);
file: ["mocha.env.js"]

And then just go about using chai-as-promised how you normally would. I just wrote up a few intentionally broken tests to make sure the matchers are working properly and everything seems fine for ESM projects at least.

import { expect } from "chai";

describe("promise tests", () => {
  it("should succeed", async () => {
    await expect(Promise.resolve(10)).to.eventually.eq(10);
  });

  it("should catch rejections", async () => {
    await expect(Promise.reject(new Error("Oh no!"))).to.eventually.be.rejectedWith("Oh no!");
  });

  it("should fail", async () => {
    await expect(Promise.resolve(10)).to.eventually.eq(20);
  });

  it("should fail to catch rejections", async () => {
    await expect(Promise.reject(new Error("Oh no!"))).to.eventually.be.rejectedWith("Oops!");
  });
});
  promise tests
    ✔ should succeed
    ✔ should catch rejections
    1) should fail
    2) should fail to catch rejections

@perrin4869
Copy link

yeah, I expect the code is compatible, and if you force it, npm will allow you to run chai-as-promised with chai@5, but it's not allowed as per chai-as-promised with the peer dependencies it has set.

One way we could go about this change is to make all assertions return a Promise, but that's a big breaking change (though arguably one that gives us future flexibility for all kinds of async stuff).

I don't like this idea at all to be honest 😅
But having built in constructs to run assertions on promises seems like a good candidate to be built in to be honest, considering that at this point, Promises are the base of all async JS APIs.

@mscharley
Copy link

yeah, I expect the code is compatible, and if you force it, npm will allow you to run chai-as-promised with chai@5, but it's not allowed as per chai-as-promised with the peer dependencies it has set.

Yeah, I did notice that after I posted but didn't know what to make of it. npm definitely didn't complain at all for me. npm peer dependencies working as intended yet again, I guess. I'd definitely still prefer to see it in chai directly, but maybe a short-term solution is just republishing the original package with an updated peer dependency so that people aren't blocked while a longer term solution is worked out.

@43081j
Copy link
Contributor

43081j commented May 3, 2024

so it sounds like chai-as-promised just needs the peer dependency semver changing to include 5?

im not sure how active @domenic is anymore in that repo, so we might struggle to update it, but could be worth opening a PR

ultimately, i think this issue still stands as we should really natively support promises in chai IMO

@domenic
Copy link
Contributor

domenic commented May 4, 2024

I'd be happy to transfer the chai-as-promised repo to the chaijs GitHub organization and the same for the npm package.

@keithamus
Copy link
Member

Thanks @domenic we’d be happy to take it! I’ve invited you to the GitHub org (I actually thought you were already a member) so you can initiate the transfer.

@keithamus
Copy link
Member

Thanks @domenic for transferring the repo! If you have time could you please also go to https://www.npmjs.com/package/chai-as-promised/access and invite the user https://www.npmjs.com/~chaijs to be able to publish packages. This account is the account we use for publishing all chai packages. Thanks!

@perrin4869
Copy link

Sorry for the (mostly) unrelated comment, but I noticed that @domenic is also the maintainer of sinon-chai, which I'm also using. Would that plugin also be welcome in this org? I could also help maintain it if there is not enough throughput

@keithamus
Copy link
Member

Happy to house any and all chai plugins under the org, existing maintainers remain as such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants