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 _isNativeReflectConstruct helper #8461

Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Aug 13, 2018

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

Date#toString is generic in ES2015 1 and it doesn't throw, but it returns "Invalid Date"

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Aug 13, 2018
@nicolo-ribaudo nicolo-ribaudo force-pushed the isNativeReflectConstruct-generic-date branch from d6cf6ab to 3d7873e Compare August 13, 2018 05:41
@babel-bot
Copy link
Collaborator

babel-bot commented Aug 13, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/41855/

@JLHwung JLHwung self-requested a review November 14, 2019 16:49
// 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() {}));
Copy link
Contributor

@JLHwung JLHwung Nov 14, 2019

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.

@JLHwung JLHwung changed the title Fix _isNaiveReflectConstruct helper Fix _isNativeReflectConstruct helper Nov 14, 2019
Date#toString is generic in ES2015 [1] and it doesn't throw, but it returns "Invalid Date"

[1]: tc39/ecma262#1268 (comment)
@nicolo-ribaudo nicolo-ribaudo force-pushed the isNativeReflectConstruct-generic-date branch from 3d7873e to 72fa863 Compare February 21, 2021 22:49
@nicolo-ribaudo nicolo-ribaudo changed the base branch from master to main February 21, 2021 22:49
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 21, 2021

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:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

!Reflect.construct ||
// core-js@3
Reflect.construct.sham
) return false;
Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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";
Copy link
Member

Choose a reason for hiding this comment

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

@nicolo-ribaudo nicolo-ribaudo merged commit 771841f into babel:main Feb 22, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the isNativeReflectConstruct-generic-date branch February 22, 2021 21:49
This was referenced Mar 13, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isNativeReflectConstruct may not be always work
4 participants