Replace array indexOf checks with includes

[bendtherules]

Closes #898

Changes:

Replaced indexOf calls to check presence or absence with includes (with negation operator when checking absence).

Decisions:

1. There are some indexOf calls which use the index value to both check presence and use it to remove some elements at that position. They can be potentially refactored with separate calls to includes and indexOf, but I decided not to do that as indexOf alone can serve both the purposes here.

2. In testcases, replaced assert.equal + indexOf check with -1 by assert.ok + includes. This will probably show less information during error, as the actual index value won't be printed. That seems like a small price to pay.

Observations:

1. Most of the tests don't seem to fail if I change the presence or absence logic (includes or !includes) with the opposite one. I often did this mistake when working on this initially, but none of the test cases failed as they should have.
   Maybe we should add some more tests around that?

[Gerrit0]

I'll need to take a closer look at the tests, since that makes it sound like something else is broken, but otherwise this looks great! Thanks!

[aciccarello]

@bendtherules Thanks for taking this up. I agree the const index = [].indexOf(); if (index !== -1) [].splice(index, 1) should remain as is. I wonder if there are any more instances that would make sense to change. See the files I found below and let me know if there is a reason we should keep those as is.

• ComentPlugin.ts:66
• DynamicModulePlugin.ts:57
• converter/converter.ts:315

I also want to be careful about handling indexOf(undefined) since it isn't the same with includes. Hopefully we aren't dealing with arrays with holes but it's worth mentioning.

[bendtherules]

See the files I found below and let me know if there is a reason we should keep those as is.

• ComentPlugin.ts:66
• DynamicModulePlugin.ts:57
• converter/converter.ts:315

Thanks for noticing these. I tried to looked through Array.indexOf references, but not for other iterables. Have fixed this.

[bendtherules]

In test/utils.path.ts:22 :

    it('Minimatch can match absolute paths expressions', () => {
      const paths = ['/unix/absolute/**/path', '\\windows\\alternative\\absolute\\path', 'C:\\Windows\\absolute\\*\\path', '**/arbitrary/path/**'];
      const mms = createMinimatch(paths);
      const patterns = mms.map(({ pattern }) => pattern);
      const comparePaths = [
        absolutePath('/unix/absolute/**/path'),
        absolutePath('/windows/alternative/absolute/path'),
        absolutePath('/Windows/absolute/*/path'),
        '**/arbitrary/path/**'
      ];

      Assert(isEqual(patterns, comparePaths), `Patterns have been altered:\nMMS: ${patterns}\nPaths: ${comparePaths}`);

Here the third path 'C:\\Windows\\absolute\\*\\path' might not match with absolutePath('/Windows/absolute/*/path'), if the project directory is not in C drive. This is probably not going to be a problem in CI system, but can fail for other developers using a different drive letter.

This is failing in my local Windows setup. We might want to change it somehow (or make this optional?).
Maybe also setup some WIndows CI like Appveyor for later edge cases?

[bendtherules]

Hi @aciccarello , do this changes look good?

[aciccarello]

Sorry for the delay. These changes look great!

If you are having trouble running the tests on windows, could you please open another issue so we can get that fixed separately. Thanks!