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 _isNativeReflectConstruct helper #8461
Fix _isNativeReflectConstruct helper #8461
Conversation
nicolo-ribaudo
commented
Aug 13, 2018
•
edited
edited
Q | A |
---|---|
Fixed Issues? | Fixes #8419 |
Patch: Bug Fix? | |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
d6cf6ab
to
3d7873e
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/41855/ |
// check Map#get (which on the other hand doesn't exist in ES5 browsers). | ||
// | ||
// [1]: https://github.com/tc39/ecma262/issues/1268#issuecomment-410104832 | ||
|
||
Date.prototype.toString.call(Reflect.construct(Date, [], 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.
Per 20.3.4.41, if Reflect.construct(Date, [], function() {})
is not a Date object, this line must returns "Invalid Date"
, thus it suffices to check if it does not return "Invalid Date".
It helps shrink the code size a bit as it is included in the wrapNativeSuper
helpers.
Date#toString is generic in ES2015 [1] and it doesn't throw, but it returns "Invalid Date" [1]: tc39/ecma262#1268 (comment)
3d7873e
to
72fa863
Compare
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit cc85d4e:
|
!Reflect.construct || | ||
// core-js@3 | ||
Reflect.construct.sham | ||
) return false; |
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.
Weak Nit: I actually liked the previous style better.
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.
Me too, but I noticed that merging them minifies better (terser doesn't merge the two if statements).
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.
It should, I made the PR myself 😉 : terser/terser#555
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.
Not with this code 🤔
// write or paste code here
export function f() {
if (!a) return false;
if (!b) return false;
try {
foo();
return true;
} catch {
return false;
}
}
(tested at https://try.terser.org/)
Anyway, I'm happy to revert this change since it will be eventually handled by terser.
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.
Ahh, it's because it can't collapse the try-catch statement into an expression, so the if-statements exist only as a series of if-statements and not a nested ternary. Open an issue?
// Date object. | ||
// [1]: https://github.com/tc39/ecma262/issues/1268#issuecomment-410104832 | ||
|
||
return Date.prototype.toString.call(Reflect.construct(Date, [], function() {})) !== "Invalid Date"; |
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.
We could also change this to Boolean.prototype.valueOf
: