Bind: Add Array.includes to bind by kmh287 · Pull Request #7401 · ampproject/amphtml

kmh287 · 2017-02-07T17:23:36Z

Adds "Array.includes" to bind. Also adds a polyfill for Array.includes.

kmh287 · 2017-02-07T17:24:15Z

/to @choumx @jridgewell

jridgewell

Need tests, explicitly:

NaN should find NaN
-0 and 0 should find either.
undefined should find undefined, not null
null should find null, not undefined
Any number should find itself
Any string should find itself
Any boolean should find itself
Any object should find explicitly itself

jridgewell · 2017-02-07T21:58:08Z

src/polyfills/array.js

+export function install(win) {
+  if (!win.Array.prototype.includes) {
+    /*eslint "no-extend-native": 0*/
+    Object.defineProperty(Array.prototype, 'includes', {value: includes});


Just use simple assignment.

jridgewell · 2017-02-07T22:00:52Z

src/polyfills/array.js

+    throw new TypeError('"this" is null or not defined');
+  }
+  const o = Object(this);
+  const len = o.length >>> 0;


Needless cast (>>> 0).

Replaced with your version

jridgewell · 2017-02-07T22:04:02Z

src/polyfills/array.js

+  const n = fromIndex | 0;
+  let k = Math.max(n >= 0 ? n : len - Math.abs(n), 0);
+  while (k < len) {
+    if (o[k] === searchElement) {


This will fail [NaN].includes(NaN)

Replaced with your version

jridgewell · 2017-02-07T22:08:48Z

src/polyfills/array.js

+ * @param {number} fromIndex
+ * @returns {boolean}
+ */
+export function includes(searchElement, fromIndex) {


So, we can optimize a bit:

function includes(value, fromIndex) { if (value === value) { // Everything but NaN return this.indexOf(value, fromIndex) > -1; } let i = Math.max(n >= 0 ? n : len + n, 0); for (; i < this.length; i++) { const value = this[i]; if (value !== value) { return true; } } return false; }

kmh287 · 2017-02-08T15:28:24Z

Added tests

jridgewell · 2017-02-08T18:49:52Z

src/polyfills/array.js

+* @param {!Window} win
+*/
+export function install(win) {
+  if (!win.Array.includes) {


Array.prototype.includes. What's there now is a static method on Array.

jridgewell · 2017-02-08T18:50:18Z

src/polyfills.js

 import {install as installMathSign} from './polyfills/math-sign';
 import {install as installObjectAssign} from './polyfills/object-assign';
 import {install as installPromise} from './polyfills/promise';
+import {install as installArray} from './polyfills/array';


Let's rename the file to array-includes to match the other method polyfills.

jridgewell · 2017-02-08T18:51:31Z

src/polyfills/array.js

+ */
+export function includes(value, fromIndex) {
+  /* eslint no-self-compare: 0 */
+  if (value === value) { // Everything but NaN


Thinking about this more, let's drop this and push all cases into the for loop. We'll need to change it a bit:

const other = this[i]; if (other === value || (value !== value && other !== other)) { return true; }

dreamofabear · 2017-02-08T19:07:43Z

src/polyfills/array.js

+/**
+ * Returns true if the element is in the array and false otherwise.
+ *
+ * @param {*} searchElement


s/searchElement/value

dreamofabear · 2017-02-08T19:08:39Z

src/polyfills/array.js

+
+
+/**
+ * Returns true if the element is in the array and false otherwise.


Perhaps add an @see annotation with the MDN link.

Since we're not using that version anymore, I don't think that'll be necessary.

dreamofabear · 2017-02-08T19:09:49Z

test/functional/test-polyfill-array.js

+
+describe('Array.includes', () => {
+
+  const arrayWithPrimitives = [false, 17, 'hello world'];


Move this into the it() method. Ditto for other tests below.

dreamofabear · 2017-02-08T19:11:01Z

src/polyfills/array.js

+ * @param {number} fromIndex
+ * @returns {boolean}
+ */
+export function includes(value, fromIndex) {


Does this need to be exported? If not, you can use a mock Window object with install() below to access it for testing.

dreamofabear · 2017-02-08T19:12:39Z

src/polyfills/array.js

+  }
+  fromIndex = fromIndex || 0;
+  const len = this.length;
+  let i = Math.max(fromIndex >= 0 ? fromIndex : len + fromIndex, 0);


You only need to max the false branch of the ternary, right? I think that would be a bit more readable too.

dreamofabear · 2017-02-08T19:14:04Z

src/polyfills/array.js

+  fromIndex = fromIndex || 0;
+  const len = this.length;
+  let i = Math.max(fromIndex >= 0 ? fromIndex : len + fromIndex, 0);
+  for (; i < len; i++) {


Any reason why you're declaring i outside the loop?

Don't want the line to be too long

dreamofabear · 2017-02-08T20:41:51Z

src/polyfills/array-includes.js

+ * @returns {boolean}
+ */
+function includes(value, fromIndex) {
+  /* eslint no-self-compare: 0 */


Is this still necessary?

dreamofabear · 2017-02-08T20:44:35Z

src/polyfills/array-includes.js

+ * Returns true if the element is in the array and false otherwise.
+ *
+ * @param {*} value
+ * @param {number} fromIndex


For optional params:

@param {number=} opt_fromIndex

dreamofabear · 2017-02-08T20:49:17Z

src/polyfills/array-includes.js

+ */
+function includes(value, fromIndex) {
+  /* eslint no-self-compare: 0 */
+  fromIndex = fromIndex || 0;


@jridgewell Do we support default parameters yet? Looks like Babel/Closure has basic support.

dreamofabear · 2017-02-08T20:51:02Z

src/polyfills/array-includes.js

+  let i = fromIndex >= 0 ? fromIndex : Math.max(len + fromIndex, 0);
+  for (; i < len; i++) {
+    const other = this[i];
+    if (other === value || (value !== value && other !== other)) {


Can you add a comment explaining value !== value?

…ontain

jridgewell

I think a nice next step is to find anywhere that does Arary#indexOf > -1 (or one of the == -1, >= 0, < 0) and update them to #includes. You'll also need to remove our Array#includes ban.

jridgewell · 2017-02-09T17:23:16Z

src/polyfills.js

 import {install as installMathSign} from './polyfills/math-sign';
 import {install as installObjectAssign} from './polyfills/object-assign';
 import {install as installPromise} from './polyfills/promise';
+import {install as installArray} from './polyfills/array-includes';


Nit: rename to installArrayIncludes

kmh287 · 2017-02-09T18:17:57Z

Can do, but would prefer not to clutter this PR.

jridgewell · 2017-02-09T18:45:34Z

Can do, but would prefer not to clutter this PR.

Yah, I meant as a follow up.

dreamofabear · 2017-02-09T19:02:35Z

Looks good. Last thing: can you check the impact to file size for v0.js?

jridgewell · 2017-02-09T19:13:57Z

src/polyfills/array-includes.js

+ *
+ * @param {*} value
+ * @param {number=} opt_fromIndex
+ * @returns {boolean}


Nit @return

kmh287 · 2017-02-10T20:43:18Z

+222 bytes

…ontain

kmh287 · 2017-02-10T21:42:48Z

Fixes #7378

* add array polyfill, whitelist contains function * install the polyfill * debuggign test * typos * cleanup * cleanup * contains -> includes * more cleanup * forgot to update one contains to includes * whitelist * fix failing test * pr comments * added test * fixed slight issue with the polyfill * add test * typo * pr comments * cleanup * cleanup * pr comments * ci issues * more ci issues * even more ci issues * update whitelist * pr comments * remove ban on includes * pr comments

kmh287 added 6 commits February 7, 2017 10:51

add array polyfill, whitelist contains function

b3354b4

install the polyfill

1a5f5da

debuggign test

e8950af

typos

0ea90e9

cleanup

7f1a0bf

cleanup

83c67b8

kmh287 added 2 commits February 7, 2017 13:40

contains -> includes

6401283

more cleanup

91bc0e4

kmh287 changed the title ~~Bind: Add Array.contains to bind~~ Bind: Add Array.includes to bind Feb 7, 2017

kmh287 added 2 commits February 7, 2017 14:20

forgot to update one contains to includes

11def87

whitelist

ef1f5d1

dreamofabear requested review from dreamofabear and jridgewell February 7, 2017 20:19

fix failing test

c4c0146

jridgewell requested changes Feb 7, 2017

View reviewed changes

kmh287 added 4 commits February 8, 2017 09:20

pr comments

d9df09e

added test

a5202b9

fixed slight issue with the polyfill

0cc1bfc

add test

3c3ddb9

typo

66ce110

jridgewell reviewed Feb 8, 2017

View reviewed changes

dreamofabear reviewed Feb 8, 2017

View reviewed changes

kmh287 added 3 commits February 8, 2017 14:42

pr comments

4aa660b

cleanup

ae5c71e

cleanup

8c85141

dreamofabear reviewed Feb 8, 2017

View reviewed changes

kmh287 added 2 commits February 8, 2017 18:43

Merge branch 'master' of github.com:kmh287/amphtml into #7378_array_c…

8ea2b5e

…ontain

pr comments

66bd5cf

kmh287 and others added 4 commits February 8, 2017 18:58

ci issues

fefe589

more ci issues

e2de3c3

even more ci issues

6de57b4

update whitelist

34dde4a

jridgewell approved these changes Feb 9, 2017

View reviewed changes

pr comments

7e692b1

remove ban on includes

3d945ec

jridgewell reviewed Feb 9, 2017

View reviewed changes

kmh287 added 2 commits February 9, 2017 15:16

pr comments

7fe0dad

merge master

a12c6eb

Merge branch 'master' of github.com:kmh287/amphtml into #7378_array_c…

6935240

…ontain

dreamofabear approved these changes Feb 10, 2017

View reviewed changes

jridgewell merged commit cce7bda into ampproject:master Feb 10, 2017

jridgewell mentioned this pull request Feb 14, 2017

Add support for Array.includes to amp-bind #7378

Closed

This was referenced Mar 20, 2017

includes is not available for amp-bind #8251

Closed

Update amp-bind.md #8309

Merged



		/**
		* Returns true if the element is in the array and false otherwise.


		describe('Array.includes', () => {

		const arrayWithPrimitives = [false, 17, 'hello world'];

Conversation

kmh287 commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmh287 commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmh287 commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmh287 commented Feb 9, 2017

Uh oh!

jridgewell commented Feb 9, 2017

Uh oh!

dreamofabear commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

kmh287 commented Feb 7, 2017 •

edited

Loading

kmh287 commented Feb 7, 2017 •

edited

Loading