Core: Remove inArray with native array.includes()

[izelnakri]

While reading the source code I realized this internal function isn't needed anymore if we drop the IE support:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes#browser_compatibility

[Krinkle]

@izelnakri We've discussed raising Node.js requirements for for QUnit 3, but not yet browser support. I think we could consider dropping support for IE10 and below (depends on the benefit, I'd still prefer we include a fallback and reap the perf benefits at runtime and accept increases in QUnit transfer size). However jQuery, WordPress, and Wikimedia all still support IE11 and even if they drop it today it will be a few years before their LTSes expire where I'd prefer to offer them latest QUnit as well (instead of e.g. maintaining support for QUnit 2.x as I'd prefer not to maintain multiple branches).

I'm confident that apart from certain startup costs, any major runtime perf benefits we should be able to achieve even with a fallback in place.

For this one, we could conditionally define inArray between two implementations and thus not pay for the check at runtime.

[izelnakri]

@Krinkle I agree with your points, in a new PR I introduced significant performance improvements by assuming non-IE browser support(particularly using Array.from, Array.prototoype.some, Array.prototype.every): https://github.com/qunitjs/qunit/pull/1701

This could be a strong case for an optional polyfill inclusion, lets discuss it in this thread if you like ;)

[Krinkle]

This could be a strong case for an optional polyfill inclusion, lets discuss it in this thread if you like ;)

No problem. Let's take a sidebar here.

The topic of polyfills has come up before. I've generally been of the opinion that we shouldn't embed polyfills in QUnit because it changes global state. This is particularly problematic at the unit test layer when QUnit is used by projects such as jQuery and Lodash (which are to some extent kind a polyfill themselves, to normalise differences between browsers), and even actual polyfill projects like core-js (which uses QUnit) or es5-shim and FT polyfill-library (which use Karma with a different unit testing framework).

Having said that, we do have a lib/ directory that holds promise-polyfill.js, and we embed kleur from npm for example. So long as there is a way to import these cleanly, we can certainly use them for more complex polyfills where a little snippet in utilities.js might feel too much to maintain.

[izelnakri]

I just added a file for simple array polyfills, however it is not imported/used by any of the qunit code yet. Feel free to edit the build system/files to add it to runtime anyway you like.

Also I added a note for Array.from, this polyfill seems to be complex and we might want to use an npm package like array.from, what do you think? I left a TODO comment for it for now.

[Krinkle]

@izelnakri

If the take the same approach here in utilities as we took before (for StringMap, globalThis, and performance) than the result would effectively be what src/core/utilities.js already is. It's possible that the native method is found faster even if it is called indirectly, then you could use something like the following to define it conditionally. That way we only pay the price once at init and not at runtime, even though it is still an extra function call (as our code would continue to call utilities.inArrray).

Something like this:

- export function inArray (elem, array) {
-  return array.indexOf(elem) !== -1;
- } 
+ export const inArray = Array.prototype.includes
+   ? (elem, array) => array.includes(elem)
+   : (elem, array) => array.indexOf(elem) !== -1;

I don't see a way to incorporate a polyfill directly, but either way it seems simpler to maintain inline I think. I'd be fine with additional ones being added like this if you see a use for other ES6+ array methods that some browsers don't support.

I noticed that the polyfill file in this PR also includes Array#every and Array#some. Those were alreday part of ES5 supported in all browsers we support (ES5, IE9+). There is a few places in QUnit that already call Array#some in fact!

[izelnakri]

Hi @Krinkle ! It's been 5 months since my last comment so it took me a while to get the context :sweat_smile: I agree with both points and making the adjustments now.

[izelnakri]

I made the requested changes, while implementing I remembered one of my intentions were to keep the code more readable by removing the inArray wrapper references.

We could achieve this by:

Array.prototype.includes = Array.prototype.includes || function (elem) { 
  return this.indexOf(elem) !== -1;
};

and then remove the inArray references. What do you think?

[Krinkle]

@izelnakri

That's how I'd do it in any regular web app indeed, but we can't in a testing library like QUnit. As per https://github.com/qunitjs/qunit/pull/1695#issuecomment-1218252847, doing so leaks into global state, thus changing or making impossible the testing of libraries that themselves involve such conditions. E.g. if you maintian a library that does what we do in QUnit (foo ? Array.prototype.includes : bar), or better yet, a project that provides such polyfills, then we'd be invalidating the test coverage you get from browsers that normally excercise that condition, possibly even making the developer's code think that it is inside a modern browser and breaking other things.

[izelnakri]

@Krinkle I see. Although I don't see this being practically as important given the probability and the state of this issue, I'm willing to keep it as it is and not do it :) Let me know if anything else is needed, cheers!