Make Array includes() polyfill not enumerable#7517
Make Array includes() polyfill not enumerable#7517aghassemi merged 2 commits intoampproject:masterfrom
includes() polyfill not enumerable#7517Conversation
|
Who on earth is using a for-in on an array? If we do this, let's update the other polyfills. |
src/polyfills/array-includes.js
Outdated
| if (!win.Array.prototype.includes) { | ||
| win.Array.prototype.includes = includes; | ||
| win.Object.defineProperty(Array.prototype, 'includes', { | ||
| enumerable: false, |
There was a problem hiding this comment.
enumerable can be omitted, since it defaults to false. However, we should include writable: true and configurable: true.
There was a problem hiding this comment.
What's the use-case for making it writable and configurable?
There was a problem hiding this comment.
It matches the normal methods. Plus this is just confusing:
// Setup
var Arr = Object.create(Array.prototype, { includes: { value: Array.prototype.includes } });
var a = Object.create(Arr);
// Surprise!
a.includes = 'test';
a.includes; // => function includes() { [native code] }There was a problem hiding this comment.
huh, you are right, native includes is writable and configurable. Did not expect that...
|
@jridgewell PTAL, I am keeping |
|
Thanks for fixing this! |
|
The following tests are failing on travis after this Not sure whether these are flakes. |
|
https://travis-ci.org/ampproject/amphtml/builds/201349924 Currently the only failure is |
|
Yeah, it's a flake :) |
All the
for var in arrcode that did not checkhasOwnPropertyhas been broken since the polyfill did not makeincludesnot enumerable.Fixes Chrome 45 tests failures on Master as well.
/to @jridgewell