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

Wrongly added Array.prototype.keys polyfill when Object.keys is used #2448

Closed
lahmatiy opened this issue Apr 21, 2017 · 8 comments
Closed

Wrongly added Array.prototype.keys polyfill when Object.keys is used #2448

lahmatiy opened this issue Apr 21, 2017 · 8 comments
Assignees

Comments

@lahmatiy
Copy link

lahmatiy commented Apr 21, 2017

When Object.keys() is used, closure compiler adds Array.prototype.keys polyfill. Test file:

function test(o) {
    return Object.keys(o);
}

Output:

var $jscomp=$jscomp||{};$jscomp.scope={};$jscomp.defineProperty="function"==typeof Object.defineProperties?Object.defineProperty:function(a,c,b){if(b.get||b.set)throw new TypeError("ES3 does not support getters and setters.");a!=Array.prototype&&a!=Object.prototype&&(a[c]=b.value)};$jscomp.getGlobal=function(a){return"undefined"!=typeof window&&window===a?a:"undefined"!=typeof global&&null!=global?global:a};$jscomp.global=$jscomp.getGlobal(this);$jscomp.SYMBOL_PREFIX="jscomp_symbol_";
$jscomp.initSymbol=function(){$jscomp.initSymbol=function(){};$jscomp.global.Symbol||($jscomp.global.Symbol=$jscomp.Symbol)};$jscomp.symbolCounter_=0;$jscomp.Symbol=function(a){return $jscomp.SYMBOL_PREFIX+(a||"")+$jscomp.symbolCounter_++};
$jscomp.initSymbolIterator=function(){$jscomp.initSymbol();var a=$jscomp.global.Symbol.iterator;a||(a=$jscomp.global.Symbol.iterator=$jscomp.global.Symbol("iterator"));"function"!=typeof Array.prototype[a]&&$jscomp.defineProperty(Array.prototype,a,{configurable:!0,writable:!0,value:function(){return $jscomp.arrayIterator(this)}});$jscomp.initSymbolIterator=function(){}};$jscomp.arrayIterator=function(a){var c=0;return $jscomp.iteratorPrototype(function(){return c<a.length?{done:!1,value:a[c++]}:{done:!0}})};
$jscomp.iteratorPrototype=function(a){$jscomp.initSymbolIterator();a={next:a};a[$jscomp.global.Symbol.iterator]=function(){return this};return a};$jscomp.iteratorFromArray=function(a,c){$jscomp.initSymbolIterator();a instanceof String&&(a+="");var b=0,d={next:function(){if(b<a.length){var e=b++;return{value:c(e,a[e]),done:!1}}d.next=function(){return{done:!0,value:void 0}};return d.next()}};d[Symbol.iterator]=function(){return d};return d};
$jscomp.polyfill=function(a,c,b,d){if(c){b=$jscomp.global;a=a.split(".");for(d=0;d<a.length-1;d++){var e=a[d];e in b||(b[e]={});b=b[e]}a=a[a.length-1];d=b[a];c=c(d);c!=d&&null!=c&&$jscomp.defineProperty(b,a,{configurable:!0,writable:!0,value:c})}};$jscomp.polyfill("Array.prototype.keys",function(a){return a?a:function(){return $jscomp.iteratorFromArray(this,function(a){return a})}},"es6-impl","es3");function test(a){return Object.keys(a)};

I believe it's incorrect.

> java -jar  compiler.jar --version
Closure Compiler (http://github.com/google/closure-compiler)
Version: v20170409
Built on: 2017-04-11 10:26
@MatrixFrog
Copy link
Contributor

In general we will sometimes add polyfills we don't need, though we can probably do better in this case. cc @shicks

@gaearon
Copy link
Contributor

gaearon commented Dec 11, 2017

This didn't use to be an issue for us, but after updating to 20171203, my bundle now contains this (prettified):

"use strict";
var $jscomp = $jscomp || {};
$jscomp.scope = {};
$jscomp.owns = function(obj, prop) {
  return Object.prototype.hasOwnProperty.call(obj, prop);
};
$jscomp.ASSUME_ES5 = !1;
$jscomp.ASSUME_NO_NATIVE_MAP = !1;
$jscomp.ASSUME_NO_NATIVE_SET = !1;
$jscomp.defineProperty =
  $jscomp.ASSUME_ES5 || "function" == typeof Object.defineProperties
    ? Object.defineProperty
    : function(target, property, descriptor) {
        target != Array.prototype &&
          target != Object.prototype &&
          (target[property] = descriptor.value);
      };
$jscomp.getGlobal = function(maybeGlobal) {
  return "undefined" != typeof window && window === maybeGlobal
    ? maybeGlobal
    : "undefined" != typeof global && null != global ? global : maybeGlobal;
};
$jscomp.global = $jscomp.getGlobal(this);
$jscomp.polyfill = function(target, polyfill) {
  if (polyfill) {
    var obj = $jscomp.global;
    target = target.split(".");
    for (var i = 0; i < target.length - 1; i++) {
      var key = target[i];
      key in obj || (obj[key] = {});
      obj = obj[key];
    }
    target = target[target.length - 1];
    i = obj[target];
    polyfill = polyfill(i);
    polyfill != i &&
      null != polyfill &&
      $jscomp.defineProperty(obj, target, {
        configurable: !0,
        writable: !0,
        value: polyfill
      });
  }
};
$jscomp.polyfill(
  "Object.assign",
  function(orig) {
    return orig
      ? orig
      : function(target, var_args) {
          for (var i = 1; i < arguments.length; i++) {
            var source = arguments[i];
            if (source)
              for (var key in source)
                $jscomp.owns(source, key) && (target[key] = source[key]);
          }
          return target;
        };
  },
  "es6",
  "es3"
);
$jscomp.SYMBOL_PREFIX = "jscomp_symbol_";
$jscomp.initSymbol = function() {
  $jscomp.initSymbol = function() {};
  $jscomp.global.Symbol || ($jscomp.global.Symbol = $jscomp.Symbol);
};
$jscomp.Symbol = (function() {
  var counter = 0;
  return function(opt_description) {
    return $jscomp.SYMBOL_PREFIX + (opt_description || "") + counter++;
  };
})();
$jscomp.initSymbolIterator = function() {
  $jscomp.initSymbol();
  var symbolIterator = $jscomp.global.Symbol.iterator;
  symbolIterator ||
    (symbolIterator = $jscomp.global.Symbol.iterator = $jscomp.global.Symbol(
      "iterator"
    ));
  "function" != typeof Array.prototype[symbolIterator] &&
    $jscomp.defineProperty(Array.prototype, symbolIterator, {
      configurable: !0,
      writable: !0,
      value: function() {
        return $jscomp.arrayIterator(this);
      }
    });
  $jscomp.initSymbolIterator = function() {};
};
$jscomp.arrayIterator = function(array) {
  var index = 0;
  return $jscomp.iteratorPrototype(function() {
    return index < array.length
      ? { done: !1, value: array[index++] }
      : { done: !0 };
  });
};
$jscomp.iteratorPrototype = function(next) {
  $jscomp.initSymbolIterator();
  next = { next: next };
  next[$jscomp.global.Symbol.iterator] = function() {
    return this;
  };
  return next;
};
$jscomp.iteratorFromArray = function(array, transform) {
  $jscomp.initSymbolIterator();
  array instanceof String && (array += "");
  var i = 0,
    iter = {
      next: function() {
        if (i < array.length) {
          var index = i++;
          return { value: transform(index, array[index]), done: !1 };
        }
        iter.next = function() {
          return { done: !0, value: void 0 };
        };
        return iter.next();
      }
    };
  iter[Symbol.iterator] = function() {
    return iter;
  };
  return iter;
};
$jscomp.polyfill(
  "Array.prototype.keys",
  function(orig) {
    return orig
      ? orig
      : function() {
          return $jscomp.iteratorFromArray(this, function(i) {
            return i;
          });
        };
  },
  "es6",
  "es3"
);

I didn't change any options.

You can reproduce by installing Yarn and then running

git clone https://github.com/gaearon/react.git
cd react
git checkout bump-gcc
yarn
yarn build core --type=FB_DEV

The build output is in build/facebook-www/React-prod.js (it's prettified for readability).

This is a blocker for updating for us: we can't afford to ship any polyfills.

@lahmatiy
Copy link
Author

lahmatiy commented Dec 13, 2017

@gaearon After I filled the issue, I found the solution a few months ago. If --no-rewritePolyfills option is passed, GCC stops adding a polyfill prelude. Not sure it still working, but maybe it helps to you too.

@gaearon
Copy link
Contributor

gaearon commented Dec 13, 2017

I’m using the JS version that claims rewritePolyfills is set to false by default. I assume it’s the exact opposite? Or maybe it’s a separate flag that isn’t exposed in JS version? Regardless, thanks for the pointer!

@gaearon
Copy link
Contributor

gaearon commented Apr 16, 2018

This is still an issue with 20180402.0.0 and is blocking us from upgrading beyond last year’s September version. Is there anything I could do to help move this forward?

@gaearon
Copy link
Contributor

gaearon commented Apr 16, 2018

Hmm. I explicitly set rewritePolyfills to false and indeed it helped.
The documentation was wrong (thanks @nazar-pc for fixing it!)

@shicks
Copy link
Member

shicks commented Dec 9, 2019

I was unable to reproduce this with the current compiler. My guess is that the type information on Object was insufficiently precise for the compiler to remove the polyfill (it adds it unconditional on type information, then fails to remove it if it can't prove it's not an Array), but that either the restructuring we did with polyfill removal a year or so ago, or else improved type information on namespaces, has since caused the issue to go away.

If this is still a problem, please reopen.

@shicks shicks closed this as completed Dec 9, 2019
@westc
Copy link

westc commented Apr 25, 2021

I am still getting this issue while using closure-compiler-v20210406.jar but I found a workaround by simply changing the code to use bracket notation:

function test(o) {
    return Object["keys"](o);
}

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

No branches or pull requests

5 participants