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

Fix 1564 #1566

Merged
merged 22 commits into from
Jan 25, 2024
Merged

Fix 1564 #1566

merged 22 commits into from
Jan 25, 2024

Conversation

koddsson
Copy link
Member

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
Copy link
Contributor

43081j commented Dec 29, 2023

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

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

ReDemoNBR commented Dec 29, 2023

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
Copy link
Member Author

koddsson commented Jan 5, 2024

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 😸

@@ -590,6 +591,56 @@ assert.isNotFunction = function (val, msg) {
new Assertion(val, msg, assert.isNotFunction, true).to.not.be.a('function');
};

/**
* TODO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

};

/**
* TODO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

lib/chai/interface/assert.js Outdated Show resolved Hide resolved
@ReDemoNBR
Copy link
Contributor

ReDemoNBR commented Jan 5, 2024

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
Copy link
Member Author

koddsson commented Jan 6, 2024

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.

test/assert.js Outdated Show resolved Hide resolved
@43081j
Copy link
Contributor

43081j commented Jan 6, 2024

  • 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
Copy link
Member Author

koddsson commented Jan 6, 2024

  • 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
Copy link
Contributor

43081j commented Jan 8, 2024

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

lib/chai/core/assertions.js Outdated Show resolved Hide resolved
lib/chai/core/assertions.js Outdated Show resolved Hide resolved
@43081j
Copy link
Contributor

43081j commented Jan 15, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

43081j commented Jan 20, 2024

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
Copy link
Member Author

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.

Copy link
Member Author

@koddsson koddsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/chai/core/assertions.js Outdated Show resolved Hide resolved
@43081j
Copy link
Contributor

43081j commented Jan 22, 2024

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
keithamus previously approved these changes Jan 22, 2024
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

test/expect.js Show resolved Hide resolved
@koddsson koddsson marked this pull request as ready for review January 24, 2024 19:41
@koddsson koddsson requested a review from a team as a code owner January 24, 2024 19:41
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

Successfully merging this pull request may close these issues.

[v5] assert.isFunction() fails with async function
4 participants