Fix 1564

[koddsson]

Fix #1564 by introducing new isAsyncFunction and isNotAsyncFunction assertions.

The thinking is that even though async functions are functions this change will allow for more granular assertions.

I'm still not sure if this is what we'd like though. Async functions are still functions so I could see a argument being made for isFunction asserting that a async function is indeed a function. Maybe we should add a isSynchronousFunction to specifically check if a function is not async and then leave isFunction to assert for both sync and async functions?

Interested in hearing thoughts.

@43081j @keithamus @ReDemoNBR

[43081j]

my preference would be that isFunction matches all types of function, which means either asserting against a union of types or reintroducing the typeof code for functions.

an isAsyncFunction and isSyncFunction seems to make sense then, like you suggested. though things could get a bit sketchy since it doesn't mean any async function, it means any syntactically async function (i.e. via the async keyword). A regular function which returns a promise is async, but isn't an AsyncFunction.

not sure how best to avoid confusion there

[ReDemoNBR]

Recently in NodeJS's new util.promisify deprecation on promisified functions there was a discussion which involves how to detect if function is already promisified.

One of the points is that a regular Function can return a Promise and thus be used like an AsyncFunction, even though it is not. On top of that, I had an issue before in a project where a transpiler was converting an AsyncFunction to a Function. So I imagine tools like Babel, Typescript, Webpack, and friends may do similar transpilations and cause some confusion.

So, my preference is that assert.isFunction(f) should assert for any type of function, which would also be good for compatibility with Chai v4. And then add new precise assertions to test for AsyncFunction, GeneratorFunction, AsyncGeneratorFunction, and SyncFunction.

[koddsson]

Yooo! I pushed some changes based off of https://github.com/chaijs/chai/pull/1566#pullrequestreview-1798856715 for early feedback. Please comment any thoughts (and prayers) if something looks off otherwise I'll continue down this road 😸

[ReDemoNBR]

I like the way the code is going and I appreciate your work on this.

From my understanding of the code right now, it looks like this in the assert API:

• assert.isFunction: changed in v5.0.0 to assert it is a Function (read it as SyncFunction) - unchanged in this PR
• assert.isCallable: new assertion to replace the old Chai v4 behavior of assert.isFunction, which checks if it can be called regardless of being Function, AsyncFunction, GeneratorFunction or AsyncGeneratorFunction
• assert.isAsyncFunction: new assertion to check if it is an AsyncFunction
• assert.isGeneratorFunction: new assertion to check if it is a GeneratorFunction

So far, to check if an object is an AsyncGeneratorFunction we could test for AsyncFunction and GeneratorFunction.

Is this right? If so, I'm in love with it :+1:

---

I believe some tests to ensure the behavior of assert.isNotFunction with Async/Generators functions would be nice. What do you guys think?

Also, I am not familiar with the internals of Chai, but does it automatically create the negation of the new assertions, i.e. assert.isNotGeneratorFunction and the other ones as well?

[koddsson]

So far, to check if an object is an AsyncGeneratorFunction we could test for AsyncFunction and GeneratorFunction.

We could add a isAsyncGeneratorFunction as well for completeness.

[43081j]

• expect(f).to.be.a.function()/f.should.be.a.function()/assert.isFunction(f) should alias a callable() assert, and so likewise not.a.function/isNotFunction should be an alias for .not.callable()

i think it is important we do this part Keith mentioned

otherwise, the various places people use function now will have different behaviour than in v4.

e.g. expect(f).to.be.a('function')

then you would just negate isAsyncFunction or whatever we call it, to check if something is a non-async func

[koddsson]

• expect(f).to.be.a.function()/f.should.be.a.function()/assert.isFunction(f) should alias a callable() assert, and so likewise not.a.function/isNotFunction should be an alias for .not.callable()

i think it is important we do this part Keith mentioned

Y'all know of a good way to do this that allows us to set the error message to say expected ${val} to be Function? I keep getting expected ${val} to be callable when aliasing. I wanted to ask before I try rigging up something custom.

[43081j]

currently we'd do it by a right? so expect(f).to.be.a('function')

if we want to make an actual chain to be to.be.a.function instead, and to.be.callable, i think you'd have to use a flag (this.flag).

or we could just word it more like to be a callable function

[43081j]

in its current state, looks like you have this:

• expect(foo).to.be.callable
• expect(foo).to.be.asyncFunction
• expect(foo).to.be.generatorFunction
• expect(foo).to.be.asyncGeneratorFunction

i do wonder if we are over complicating it. i think i would have:

• expect(foo).to.be.callable (exactly the same as the one below)
• expect(foo).to.be.a('function')
  • this one used to work but we broke it as part of our change of how we do type(val)
  • it should now be truthy for all types of function (async, generator, async generator, function)
• expect(foo).to.be.an('AsyncFunction')
  • should already work (remember an and a are the same function)

i think we should just fix up a('function') and introduce callable. i don't think we need explicit property chains for the various types of function.

you could argue then we don't need callable either (since its an alias of a('function') by then), but maybe people prefer that DX.

what do you think? sorry for any chaos im causing by picking at this 😂

[koddsson]

what do you think? sorry for any chaos im causing by picking at this 😂

No this is great! I think getting this right is important.

i do wonder if we are over complicating it.

Yeah I think we might be. When in doubt go with the most simple solution so I'm all for it. I'll take a crack at that next.

[koddsson]

expect(foo).to.be.an('AsyncFunction')

• should already work (remember an and a are the same function)

I was looking at this and this the tests just fail because we're using deep-eql in the expect assertions which results in these test failures:

     AssertionError: expected [AsyncGeneratorFunction] to be an asyncfunction

Now we could carve out a exception for functions instead of using deep-eql or make a change to deep-eql but that seems like it would go against that libraries mantra.

[43081j]

strange, that isn't what i expected. what leads to it using deep-eql?

i thought an just used a regular old strict equality check on the result of type(expr) (===)

[koddsson]

strange, that isn't what i expected. what leads to it using deep-eql?

i thought an just used a regular old strict equality check on the result of type(expr) (===)

Ah! The chai code can be a bit terse to read sometimes 😅 You're correct! I'll make a push in a minute.

[43081j]

as far as the interface goes, looks good to me i think. much simpler than introducing brand new chains for each function type IMO

be good to get keith's opinion some time though too

[koddsson]

I think I've added enough test coverage but let me know if you think I'm missing something and I'll write more :)