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

[keithamus]

I agree but I'd add that we should aim for consistency between interfaces.

IMO:

• 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()
• expect(f).to.be.callable()/f.should.be.callbable()/assert.isCallable(f) should check to see if the object can be called (i.e. typeof f == 'function'). not.callable would be != 'function.
• expect(f).to.be.an.async.function()/f.should.be.an.async.function()/assert.isAsyncFunction(f) can check be.a('AsyncFunction'). This means it should be possible to chain .a.callable.async.function() even if that's redundant. Because not is chainable, assert.isNotAsyncFunction will only test that its not async, not that it's not callable. So .a.callable.not.async.function() would pass for regular functions & generator functions, and fail for async functions (generator or otherwise).
• expect(f).to.be.a.generator.function()/f.should.be.a.generator.function()/assert.isGeneratorFunction(f) can check be.a('GeneratorFunction'), although arguably it should also pass for async generators, as they're still generator functions?
• This also means expect(f).to.be.an.async.generator.function()/f.should.be.an.async.generator.function()/assert.isAsyncGeneratorFunction(f) should work for async generators.

Perhaps the async asserts should check if the stringtag matches /^Async.*Function$/ and the generator asserts should check it matches /GeneratorFunction$/?

[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 #1566 (review) for early feedback. Please comment any thoughts (and prayers) if something looks off otherwise I'll continue down this road 😸

[koddsson]

TODO

[koddsson]

TODO

[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 👍

---

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.

[koddsson]

This is quite a bit smaller PR now I think but might need more tests for the different interfaces.

[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

[keithamus]

Generally LGTM, but could add a few more tests to prevent future regressions of this type.

[keithamus]

This LGTM 👍

[ReDemoNBR]

This LGTM

[43081j]

🚢

you have some conflicts too by the looks of it, may want to catch the branch up first