Convert foo.indexOf(...) == -1 to foo.includes() and implement an eslint rule to enforce this

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: jaws, Assigned: florian)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

+++ This bug was initially created as a clone of Bug #1329182 +++

From bug 1333324 comment #19,
::: browser/base/content/browser-thumbnails.js:96
(Diff revision 1)
>    },
>  
>    _capture: function Thumbnails_capture(aBrowser) {
>      // Only capture about:newtab top sites.
> -    if (this._topSiteURLs.indexOf(aBrowser.currentURI.spec) == -1)
> +    if (!aBrowser.currentURI ||
> +        this._topSiteURLs.indexOf(aBrowser.currentURI.spec) == -1)

Is foo.indexOf(bar) == -1 a candidate for a scripted rewrite to !foo.includes(bar)?
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8837348 [details]
Bug 1339461 - Implement new eslint rule to prefer array.includes over array.indexOf comparison with -1.

https://reviewboard.mozilla.org/r/112502/#review113906

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/prefer-array-includes.js:61
(Diff revision 1)
> +      // if (expression.right.argument.value !== 1) {
> +      //   console.log(9);
> +      //   return;
> +      // }

These comments are here for debugging purposes. The tests pass when running `node tools/lint/eslint/eslint-plugin-mozilla/tests/test-run-all.js`.

However when I run `mach eslint browser/base/content/browser-addons.js` I don't get any failures. We should be getting a failure at line 72 though.

Do you see any reason why we're not getting any failures in practice?
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
mozreview-review
Comment on attachment 8837348 [details]
Bug 1339461 - Implement new eslint rule to prefer array.includes over array.indexOf comparison with -1.

https://reviewboard.mozilla.org/r/112502/#review113932

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/prefer-array-includes.js:3
(Diff revision 2)
> +/**
> + * @fileoverview Prefer using Array.includes() instead of comparing
> +                 Array.indexOf() to -1.

.indexOf and .includes both exist on all of the following objects: array, string, TypedArray. We should ensure the wording for the description of the eslint rule and the error it produces are ok for all of these cases.

::: tools/lint/eslint/eslint-plugin-mozilla/package.json:3
(Diff revision 2)
>  {
>    "name": "eslint-plugin-mozilla",
> -  "version": "0.2.18",
> +  "version": "0.2.19",

We were already at version 0.2.20 after bug 1338585.

::: tools/lint/eslint/eslint-plugin-mozilla/tests/prefer-array-includes.js:26
(Diff revision 2)
> +
> +exports.runTest = function(ruleTester) {
> +  ruleTester.run("prefer-array-includes", rule, {
> +    valid: [
> +      "array.indexOf('foo') == 3;",
> +      "array.indexOf('foo') > 0"

Is indexOf == 0 a valid case? (I would like us to recommend .startsWith when it's a string, but I don't see a way to make a difference between an indexOf call in a string or in an array of strings :( ).

::: tools/lint/eslint/eslint-plugin-mozilla/tests/prefer-array-includes.js:29
(Diff revision 2)
> +    valid: [
> +      "array.indexOf('foo') == 3;",
> +      "array.indexOf('foo') > 0"
> +    ],
> +    invalid: [
> +      invalidCode("function a() { if(array.indexOf('foo') == -1) return; }", "=="),

All of the following are invalid (and should be tested in my opinion) :
.indexOf == -1
.indexOf === -1
.indexOf != -1
.indexOf !== -1
.indexOf >= 0
.indexOf < 0

I would include a test for:
var blah = stuff.foo.indexOf(bar) == -1;
Assignee

Comment 5

2 years ago
I think attachment 8831467 [details] is my most recent xpcshell script for automated changes.
Assignee

Comment 6

2 years ago
mozreview-review
Comment on attachment 8837348 [details]
Bug 1339461 - Implement new eslint rule to prefer array.includes over array.indexOf comparison with -1.

https://reviewboard.mozilla.org/r/112502/#review114188

Removing the review flag for now; comment 4 and comment 5 contain actionable comments.
Attachment #8837348 - Flags: review?(florian)

Comment 7

2 years ago
Given https://bugzilla.mozilla.org/show_bug.cgi?id=1343902#c8 I almost wonder if we should audit some of these to see if we should switch them over to Set()...
Placing my WIP xpcshell script here in case someone else wants to take this bug in the mean time.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Assignee

Comment 9

a year ago
Posted patch script-generated patch (obsolete) — Splinter Review
The script I used is available at https://bitbucket.org/fqueze/xpcshell-rewrites/commits/157f99c70d784e16f4f73548d1d76c1da5a7fd60

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=c609a48916e440d96a3e24d5b4c5c855af371b37&filter-tier=1 is green except for:
- dom/html/test/test_bug1823.html which I'll fix by hand in the next attachment.
- wpt failures that were on the base changeset.
Attachment #8945582 - Flags: review?(dtownsend)
Assignee

Updated

a year ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee

Comment 11

a year ago
Posted patch script-generated patch v2 (obsolete) — Splinter Review
I had missed a few files in the previous run (some exclusion rules were too large in my script, and some occurences were caught by eslint :-)).
Attachment #8945622 - Flags: review?(dtownsend)
Assignee

Updated

a year ago
Attachment #8945582 - Attachment is obsolete: true
Attachment #8945582 - Flags: review?(dtownsend)
Assignee

Comment 12

a year ago
Posted patch eslint rule (obsolete) — Splinter Review
Attachment #8945623 - Flags: review?(standard8)

Comment 13

a year ago
Please remove all changes to toolkit/components/reader/ . Those files are (a) from an upstream repo, and (b) have their own eslint file that should prevent them from causing errors here assuming exceptions are set correctly, and (c) need to work on non-ES.next environments like iOS (which is defined in the eslint file as well...) so we can't make this change there.
Flags: needinfo?(florian)
(In reply to :Gijs (lower availability until Jan 27) from comment #13)
> Please remove all changes to toolkit/components/reader/ . Those files are
> (a) from an upstream repo, and (b) have their own eslint file that should
> prevent them from causing errors here assuming exceptions are set correctly,
> and (c) need to work on non-ES.next environments like iOS (which is defined
> in the eslint file as well...) so we can't make this change there.

According to the current ESLint exclusions, and a brief look at the github repo, it is only toolkit/components/reader/Readability.js and toolkit/components/reader/JSDOMParser.js that are imported. So it seems we should still be able to apply this to the other files in the directory & sub-directories? e.g. test/head.js (which is in Florian's patch).

Comment 15

a year ago
(In reply to Mark Banner (:standard8) from comment #14)
> (In reply to :Gijs (lower availability until Jan 27) from comment #13)
> > Please remove all changes to toolkit/components/reader/ . Those files are
> > (a) from an upstream repo, and (b) have their own eslint file that should
> > prevent them from causing errors here assuming exceptions are set correctly,
> > and (c) need to work on non-ES.next environments like iOS (which is defined
> > in the eslint file as well...) so we can't make this change there.
> 
> According to the current ESLint exclusions, and a brief look at the github
> repo, it is only toolkit/components/reader/Readability.js and
> toolkit/components/reader/JSDOMParser.js that are imported. So it seems we
> should still be able to apply this to the other files in the directory &
> sub-directories? e.g. test/head.js (which is in Florian's patch).

Yes, sorry for being imprecise. That said, JSDOMParser.js and Readability.js are also both in the patch...
Assignee

Comment 16

a year ago
Ran the script again with devtools/client/debugger/new/debugger.js, toolkit/components/reader/Readability.js and toolkit/components/reader/JSDOMParser.js added to the exclusion list.
Attachment #8945756 - Flags: review?(dtownsend)
Assignee

Comment 17

a year ago
Fixed the eslint error in browser_sort_in_library.js

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d8e8606e6c6906cb26ba2e35b6dbedc2e166b79
Attachment #8945757 - Flags: review?(standard8)
Assignee

Updated

a year ago
Attachment #8945623 - Attachment is obsolete: true
Attachment #8945623 - Flags: review?(standard8)
Assignee

Updated

a year ago
Attachment #8945622 - Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8945622 - Flags: review?(dtownsend)
Attachment #8945757 - Flags: review?(standard8) → review+
Attachment #8945589 - Flags: review?(dtownsend) → review+
Attachment #8945756 - Flags: review?(dtownsend) → review+

Comment 18

a year ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a5ad1dbbf2
script-generated patch to convert foo.indexOf(...) == -1 to foo.includes(), r=Mossop.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f88a7c7be35
add an eslint rule to detect when indexOf should be replaced with includes, r=Standard8.

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d5a5ad1dbbf2
https://hg.mozilla.org/mozilla-central/rev/5f88a7c7be35
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Thanks for excluding devtools/client/debugger/new/debugger.js. 

For info we have three other webpack bundles in the same folder that should be excluded from refactors:
- devtools/client/debugger/new/parser-worker.js
- devtools/client/debugger/new/pretty-print-worker.js
- devtools/client/debugger/new/search-worker.js
(or handled upstream in https://github.com/devtools-html/debugger.html)

Is there a file we can contribute to that lists generated artifacts in m-c?
Maybe we should decide on a common suffix for generated files (.bundle.js ?) to avoid conflicts.
Flags: needinfo?(florian)
Assignee

Comment 21

a year ago
(In reply to Julian Descottes [:jdescottes][:julian] from comment #20)

> Is there a file we can contribute to that lists generated artifacts in m-c?
> Maybe we should decide on a common suffix for generated files (.bundle.js ?)
> to avoid conflicts.

Having a common suffix for generated files like this would make them easy to skip for automated rewrite scripts, so I think this is a good idea. Even better would be to not have these huge generated files at all (they are difficult to look at and take a long time to parse).
Flags: needinfo?(florian)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #20)
> Is there a file we can contribute to that lists generated artifacts in m-c?
> Maybe we should decide on a common suffix for generated files (.bundle.js ?)
> to avoid conflicts.

As a starting point, adding generated files to .eslintignore is a good start - though, if they are our generated files, then I'd expect the sources to be linted.

Note also, in bug 1366714 and bug 1433522 we're discussing having various moz.build settings/flags which would probably allow scripts to more easily ignore these types of files.

As Florian said, it'd be nice to not have them autogenerated, but I understand there's good reasons for that (if node is one of the things, I believe that's currently being discussed).
Thanks for answering! 

As soon as we can use node in the build (which is being discussed but not coming soon if I understood correctly), we'll be happy to move to having the real sources in m-c for the debugger project.

All the debugger bundles are eslintignored as part of a wide rule: devtools/client/debugger/**. So even though I said "conflicts" in my previous comment, that was inappropriate. It is not a big issue for us if the bundles are automatically refactored, the changes will be overridden by the next release that updates those bundles. The only "conflict" we had with this patch was for an "asset" file which is not bundled but simply synchronized between m-c and github.

Updated

a month ago
Duplicate of this bug: 1072889
You need to log in before you can comment on or make changes to this bug.