Using Array#includes instead Array#indexOf when needed #8743

NandoSangenetto · 2017-04-13T04:55:57Z

Changed .indexOf(var) > -1 to .includes(var) === true
Changed .indexOf(var) >= 0 to .includes(var) === true
Changed (!(array.indexOf(var) == -1) to .includes(var) === true
Changed .indexOf(var) == -1 to .includes(var) === false
Changed .indexOf(var) === -1 to .includes(var) === false
Changed .indexOf(var) < 0 to .includes(var) === false

googlebot · 2017-04-13T04:56:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

NandoSangenetto · 2017-04-13T04:58:40Z

I signed it!

googlebot · 2017-04-13T04:58:42Z

CLAs look good, thanks!

dreamofabear · 2017-04-13T23:43:25Z

/to @jridgewell

dreamofabear · 2017-04-13T23:45:12Z

3p/3p.js

@@ -154,7 +154,7 @@ export function validateSrcPrefix(prefix, src) {
 * @param {string} src
 */
 export function validateSrcContains(string, src) {
-  if (src.indexOf(string) === -1) {
+  if (src.includes(string) === false) {


Instead of if (condition === false), please use if (!condition). Likewise for the true case. It's more concise and easier to read!

dreamofabear

Thanks for taking on this issue! 👍

dreamofabear · 2017-04-13T23:46:08Z

yarn.lock

@@ -345,7 +345,7 @@ asynckit@^0.4.0:
  version "0.4.0"
  resolved "https://registry.yarnpkg.com/asynckit/-/asynckit-0.4.0.tgz#c79ed97f7f34cb8f2ba1bc9790bcc366474b4b79"

-autoprefixer@6.7.0:
+autoprefixer@6.7.0, autoprefixer@^6.0.3:


Please revert this file to keep the PR strictly relevant to the attached issue.

NandoSangenetto · 2017-04-14T02:37:55Z

Done @choumx and @jridgewell

kmh287

Thanks for working on this! Just one comment.

kmh287 · 2017-04-17T13:03:36Z

ads/holder.js

@@ -25,7 +25,7 @@ export function holder(global, data) {
  const wcl = global.context.location;
  const n = navigator.userAgent;
  let l = '&r' + Math.round((Math.random() * 10000000)) + '&h' + wcl.href;
-  if (!(n.indexOf('Safari') != -1 && n.indexOf('Chrome') == -1)) {
+  if (!(n.includes('Safari') && !n.includes('Chrome'))) {


Optional: This may be more readable as

if (!n.includes('Safari') || n.includes('Chrome')) { ... }

Yep, I agree. I'll commit this change.

jridgewell · 2017-04-17T16:41:19Z

3p/3p.js

@@ -154,7 +154,7 @@ export function validateSrcPrefix(prefix, src) {
 * @param {string} src


So everything in this file cannot be updated, because the 3p directory is run in foreign iframes. This means they will not have the Array#includes polyfill installed in them.

I see. Ok, I'll revert this file.

jridgewell · 2017-04-17T16:42:02Z

ads/alp/handler.js

@@ -73,7 +73,7 @@ export function handleClick(e, opt_viewerNavigate) {
  const fragment = 'click=' + encodeURIComponent(
      addParamToUrl(link.a.href, 'amp', '1', /* opt_addToFront */ true));
  let destination = link.eventualUrl;
-  if (link.eventualUrl.indexOf('#') == -1) {
+  if (!link.eventualUrl.includes('#')) {


String#includes is not supported in IE.

jridgewell · 2017-04-17T16:42:07Z

ads/alp/handler.js

@@ -224,7 +224,7 @@ export function getA2AAncestor(win) {
  // Not a security property. We just check whether the
  // viewer might support A2A. More domains can be added to whitelist
  // as needed.
-  if (top.indexOf('.google.') == -1) {
+  if (!top.includes('.google.')) {


String#includes is not supported in IE.

jridgewell · 2017-04-17T16:42:18Z

ads/holder.js

@@ -25,7 +25,7 @@ export function holder(global, data) {
  const wcl = global.context.location;
  const n = navigator.userAgent;
  let l = '&r' + Math.round((Math.random() * 10000000)) + '&h' + wcl.href;
-  if (!(n.indexOf('Safari') != -1 && n.indexOf('Chrome') == -1)) {
+  if (!(n.includes('Safari') && !n.includes('Chrome'))) {


String#includes is not supported in IE.

jridgewell · 2017-04-17T16:43:59Z

ads/medianet.js

@@ -132,7 +132,7 @@ function loadHBTag(global, data, publisherUrl, referrerUrl) {
    let currentParam = '';
    for (let i = 0; i < allParams.length; i++) {
      currentParam = allParams[i];
-      if (dfpParams.indexOf(currentParam) === -1 && data[currentParam]) {
+      if (!dfpParams.includes(currentParam) && data[currentParam]) {


So everything in this file cannot be updated, because the ads directory is run in foreign iframes. This means they will not have the Array#includes polyfill installed in them.

jridgewell · 2017-04-17T16:51:24Z

src/service/variable-source.js

@@ -179,7 +179,7 @@ export class VariableSource {
   * @return {!VariableSource}
   */
  setAsync(varName, asyncResolver) {
-    dev().assert(varName.indexOf('RETURN') == -1);
+    dev().assert(!varName.includes('RETURN'));


String#includes is not supported in IE.

jridgewell · 2017-04-17T16:51:36Z

src/url.js

@@ -371,7 +371,7 @@ export function getSourceUrl(url) {
      ? 'https://' + decodeURIComponent(path[3])
      : 'http://' + decodeURIComponent(domainOrHttpsSignal);
  // Sanity test that what we found looks like a domain.
-  user().assert(origin.indexOf('.') > 0, 'Expected a . in origin %s', origin);
+  user().assert(origin.includes('.'), 'Expected a . in origin %s', origin);


String#includes is not supported in IE.

jridgewell · 2017-04-17T16:51:39Z

third_party/mustache/mustache.js

@@ -386,7 +386,7 @@
      var context = this, names, index, lookupHit = false;

      while (context) {
-        if (name.indexOf('.') > 0) {
+        if (name.includes('.')) {


String#includes is not supported in IE.

jridgewell · 2017-04-17T16:52:22Z

third_party/webcomponentsjs/ShadowCSS.js

@@ -236,7 +236,7 @@ export const ShadowCSS = {
      scoped = parts.map(function(p) {
        // remove :host since it should be unnecessary
        var t = p.trim().replace(polyfillHostRe, '');
-        if (t && (splits.indexOf(t) < 0) && (t.indexOf(attrName) < 0)) {
+        if (t && (!splits.includes(t)) && (!t.includes(attrName))) {


The t.includes is String#includes. String#includes is not supported in IE.

jridgewell · 2017-04-17T16:52:39Z

validator/engine/validator.js

@@ -837,7 +837,7 @@ class ChildTagMatcher {
    this.numChildTagsSeen_++;  // Increment this first to allow early exit.
    if (childTags.childTagNameOneof.length > 0) {
      const names = childTags.childTagNameOneof;
-      if (names.indexOf(tagName) === -1) {
+      if (!names.includes(tagName)) {


Another special file that can't be updated.

NandoSangenetto · 2017-04-17T17:33:41Z

Done @jridgewell, everything should be ok now :)

jridgewell · 2017-04-17T17:33:10Z

ads/taboola.js

@@ -28,7 +28,7 @@ export function taboola(global, data) {
  // ensure we have vlid publisher, placement and mode
  // and exactly one page-type
  validateData(data, ['publisher', 'placement', 'mode',
-    ['article', 'video', 'photo', 'search', 'category', 'homepage', 'others']]);
+    ['article', 'video', 'photo', 'search', 'category', 'homepage', 'other']]);


Can't change this.

Reverted manually.

jridgewell · 2017-04-17T17:33:56Z

extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js

@@ -184,7 +184,7 @@ describes.sandboxed('amp-ad-network-adsense-impl', {}, () => {
        ampStickyAd.appendChild(element);
        fixture.doc.body.appendChild(ampStickyAd);
        return impl.getAdUrl().then(adUrl => {
-          expect(adUrl.indexOf('act=sa') >= 0).to.be.true;
+          expect(adUrl.includes('act=sa')).to.be.true;


I missed this one.

* All indexOf > -1 rmeoved * Removed all indexOf >= 0 * Replacing indexOf != -1 and == -1 * All indexOf replaced to includes * Fixing lint errors * Only arrays should use includes * Conditions more concise and easier to read * Revert yarn.lock * Last char * Condition more readable * Removing all includes from strings * Removing the last string include

NandoSangenetto added 4 commits April 13, 2017 00:48

All indexOf > -1 rmeoved

e30aef2

Removed all indexOf >= 0

c28efe3

Replacing indexOf != -1 and == -1

132a89f

All indexOf replaced to includes

baba089

NandoSangenetto and others added 3 commits April 13, 2017 02:06

Fixing lint errors

c89c812

Only arrays should use includes

3b3f2c9

Merge branch 'master' into master

5c4f025

NandoSangenetto closed this Apr 13, 2017

NandoSangenetto reopened this Apr 13, 2017

dreamofabear requested review from jridgewell and dreamofabear April 13, 2017 23:43

dreamofabear reviewed Apr 13, 2017

View reviewed changes

NandoSangenetto and others added 6 commits April 13, 2017 22:56

Conditions more concise and easier to read

5b3da1c

Merge branch 'master' of github.com:nandosangenetto/amphtml

2d0e436

Merge branch 'master' into master

32b4fa4

Revert yarn.lock

9dbf335

Merge branch 'master' of github.com:nandosangenetto/amphtml

4b1f5ba

Last char

e96617c

kmh287 reviewed Apr 17, 2017

View reviewed changes

Condition more readable

5b458a0

jridgewell requested changes Apr 17, 2017

View reviewed changes

Removing all includes from strings

3430f89

jridgewell requested changes Apr 17, 2017

View reviewed changes

Removing the last string include

36518a6

jridgewell approved these changes Apr 17, 2017

View reviewed changes

jridgewell merged commit ec2a479 into ampproject:master Apr 17, 2017

ampprojectbot added the PR use: In Beta / Experimental label Apr 24, 2017

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 1, 2017

aghassemi mentioned this pull request Jun 19, 2017

Remove use of .includes on strings #10016

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Array#includes instead Array#indexOf when needed #8743

Using Array#includes instead Array#indexOf when needed #8743

NandoSangenetto commented Apr 13, 2017 •

edited

googlebot commented Apr 13, 2017

NandoSangenetto commented Apr 13, 2017

googlebot commented Apr 13, 2017

dreamofabear commented Apr 13, 2017

dreamofabear Apr 13, 2017

dreamofabear left a comment

dreamofabear Apr 13, 2017

NandoSangenetto commented Apr 14, 2017

kmh287 left a comment

kmh287 Apr 17, 2017 •

edited

NandoSangenetto Apr 17, 2017

jridgewell Apr 17, 2017

NandoSangenetto Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

jridgewell Apr 17, 2017

NandoSangenetto commented Apr 17, 2017

jridgewell Apr 17, 2017

NandoSangenetto Apr 17, 2017

jridgewell Apr 17, 2017

NandoSangenetto Apr 17, 2017

		@@ -154,7 +154,7 @@ export function validateSrcPrefix(prefix, src) {
		* @param {string} src

Using Array#includes instead Array#indexOf when needed #8743

Using Array#includes instead Array#indexOf when needed #8743

Conversation

NandoSangenetto commented Apr 13, 2017 • edited

googlebot commented Apr 13, 2017

NandoSangenetto commented Apr 13, 2017

googlebot commented Apr 13, 2017

dreamofabear commented Apr 13, 2017

Choose a reason for hiding this comment

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NandoSangenetto commented Apr 14, 2017

kmh287 left a comment

Choose a reason for hiding this comment

kmh287 Apr 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NandoSangenetto commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NandoSangenetto commented Apr 13, 2017 •

edited

kmh287 Apr 17, 2017 •

edited