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] ES5+: Use spec‑accurate IsCallable #93

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented May 16, 2020

The IsConstructor implementation is based on the one in tc39/ecma262#1798 (comment)


Fixes #48

Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Owner

@ljharb ljharb left a 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'
Copy link
Owner

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.

Copy link
Contributor Author

@ExE-Boss ExE-Boss May 17, 2020

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.

2015/IsConstructor.js Outdated Show resolved Hide resolved
Copy link
Owner

@ljharb ljharb left a 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.

2015/IsConstructor.js Outdated Show resolved Hide resolved
2015/IsConstructor.js Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the fix/es5/use-spec-accurate-iscallable-isconstructor-implementation branch from cbdd330 to 371fc87 Compare May 18, 2020 06:31
@ExE-Boss ExE-Boss force-pushed the fix/es5/use-spec-accurate-iscallable-isconstructor-implementation branch from 371fc87 to c617543 Compare May 18, 2020 09:26
@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #93 into master will increase coverage by 0.09%.
The diff coverage is 96.03%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
2015/IsConstructor.js 94.44% <94.44%> (-5.56%) ⬇️
2016/IsConstructor.js 94.44% <94.44%> (-5.56%) ⬇️
2017/IsConstructor.js 94.44% <94.44%> (-5.56%) ⬇️
2018/IsConstructor.js 94.44% <94.44%> (-5.56%) ⬇️
2019/IsConstructor.js 94.44% <94.44%> (-5.56%) ⬇️
2015/IsCallable.js 100.00% <100.00%> (ø)
2016/IsCallable.js 100.00% <100.00%> (ø)
2017/IsCallable.js 100.00% <100.00%> (ø)
2018/IsCallable.js 100.00% <100.00%> (ø)
2019/IsCallable.js 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d959e6d...a0dc7e0. Read the comment docs.

@ljharb ljharb force-pushed the fix/es5/use-spec-accurate-iscallable-isconstructor-implementation branch from c617543 to a0dc7e0 Compare June 13, 2020 21:20
@ljharb
Copy link
Owner

ljharb commented Jun 13, 2020

I've separated out the IsConstructor fixes and pulled those into master; this PR has been rebased with just the IsCallable changes.

@ljharb ljharb changed the title [Fix] ES5+: Use spec‑accurate IsCallable and IsConstructor impls [Fix] ES5+: Use spec‑accurate IsCallable Jun 13, 2020
@zloirock zloirock mentioned this pull request Sep 14, 2021
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.

ES.isCallable not spec compliant
2 participants