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

Defer to the built-in typeof if support for symbols exists. #3218

Merged
merged 1 commit into from
Dec 29, 2015
Merged

Defer to the built-in typeof if support for symbols exists. #3218

merged 1 commit into from
Dec 29, 2015

Conversation

jdalton
Copy link
Member

@jdalton jdalton commented Dec 28, 2015

This PR addresses https://phabricator.babeljs.io/T6875 by deferring to the built-in typeof operator if support for symbols exists.

@codecov-io
Copy link

Current coverage is 84.87%

Merging #3218 into master will decrease coverage by -0.31% as of d9bf5da

@@            master   #3218   diff @@
======================================
  Files          215     215       
  Stmts        15653   15653       
  Branches      3353    3353       
  Methods          0       0       
======================================
- Hit          13334   13286    -48
- Partial        675     721    +46
- Missed        1644    1646     +2

Review entire Coverage Diff as of d9bf5da

Powered by Codecov. Updated on successful CI builds.

(function (obj) {
return obj && typeof Symbol !== "undefined" && obj.constructor === Symbol ? "symbol" : typeof obj;
});
(typeof Symbol === "function" && typeof Symbol() === "symbol")
Copy link

Choose a reason for hiding this comment

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

Forgive me for lacking context here.

Does this mean every use of typeof will call Symbol() or does this result get cached and the resulting function is used?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gnarf
I'm not a pro at Babel's helpers, I based this PR off of the extends helper, but the idea is that this is an expression that will be assigned to an identifier. So the end result would look something like

var _typeof = (typeof Symbol === "function" && typeof Symbol() === "symbol")
  ? function (obj) { return typeof obj; }
  : function (obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol ? "symbol" : typeof obj; };

So modern engines would get a _typeof helper that's only

function (obj) { return typeof obj; }

@zloirock
Copy link
Member

I can explain the problem. Without external helpers / runtime, in each file with typeof will be created a symbol. Someone can transpile node_modules too - we will have thousands of symbols even without the use of symbols. Almost Symbol polyfills based on accessors in Object.prototype. Thousands useless accessors in Object.prototype can reduce performance in old engines.

Ok, how we can solve this problem?

We can add mark to the Symbol polyfill:

(typeof Symbol === "function" && !Symbol._shim) ? foo : bar;

Or we can cache the result of this detection:

(typeof Symbol === "function" && !('_shim' in Symbol ? Symbol._shim : (Symbol._shim = typeof Symbol() != "symbol"))) ? foo : bar;

@jdalton
Copy link
Member Author

jdalton commented Dec 28, 2015

Changed the check to

(typeof Symbol === "function" && typeof Symbol.iterator === "symbol")

@zloirock
Copy link
Member

It makes sense. LGTM.

@ljharb
Copy link
Member

ljharb commented Dec 28, 2015

Every engine with native Symbols implements Symbol.iterator without exception, so this LGTM also!

@jdalton
Copy link
Member Author

jdalton commented Dec 28, 2015

@ljharb Yep, I did a quick scan of @kangax's compat table before I amended the commit \o/

hzoo added a commit that referenced this pull request Dec 29, 2015
Defer to the built-in `typeof` if support for symbols exists.
@hzoo hzoo merged commit dc17966 into babel:master Dec 29, 2015
@hzoo
Copy link
Member

hzoo commented Dec 29, 2015

Looks good - thanks!

@jdalton jdalton deleted the _typeof branch December 30, 2015 02:27
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants