-
-
Notifications
You must be signed in to change notification settings - Fork 30
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] ES5+
: Use spec‑accurate IsCallable
#93
base: main
Are you sure you want to change the base?
[Fix] ES5+
: Use spec‑accurate IsCallable
#93
Conversation
Co-authored-by: Jordan Harband <ljharb@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on which specific tests wouldn't have passed before this PR?
If the only effect is that class
constructors are going to report as being callable, i don't think tightly adhering to the spec in this case is worth it.
module.exports = require('is-callable'); | ||
module.exports = function IsCallable(argument) { | ||
// some older engines say that typeof /abc/ === 'function', | ||
return typeof argument === 'function' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't actually useful though, because it will report class
constructors as callable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that’s what the specification for IsCallable
says to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this, but we'd still need to determine if getters are supported, in case Reflect.contruct is polyfilled in ES5.
cbdd330
to
371fc87
Compare
371fc87
to
c617543
Compare
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 90.90% 91.00% +0.09%
==========================================
Files 647 647
Lines 8961 9071 +110
Branches 2120 2137 +17
==========================================
+ Hits 8146 8255 +109
- Misses 815 816 +1
Continue to review full report at Codecov.
|
c617543
to
a0dc7e0
Compare
I've separated out the IsConstructor fixes and pulled those into master; this PR has been rebased with just the IsCallable changes. |
ES5+
: Use spec‑accurate IsCallable
and IsConstructor
implsES5+
: Use spec‑accurate IsCallable
The
IsConstructor
implementation is based on the one in tc39/ecma262#1798 (comment)Fixes #48