Page MenuHomePhabricator

Consider checking for NodeList.prototype.forEach as part of Grade A support or providing a polyfill
Closed, DeclinedPublic

Description

We've had several of incidents recently (T298910, T283252, T293402) due to the use of forEach on NodeList objects.
https://caniuse.com/mdn-api_nodelist_foreach

Annoyingly these older browsers tend to give useless stack traces making it hard to debug such issues if you are not looking out for them and aware of recent deploys.

I'd like to suggest we check specificially for this as part of defining Grade A support or at least provide a polyfill.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

for ES6 modules/grade A experience do we test for browser support for NodeList.prototype.forEach ?

Not explicitly, no. […]

Looking at caniuse for NodeList forEach, it seems to me that all browsers that pass these checks also support NodeList forEach, so I don't think it's necessary to add a check for it.

NodeList is a Web API, not a JavaScript API. It doesn't relate to which JS engine the browser uses, or which ES spec it follows. As such, this would imho be confusing if conflated with the es6 filter in the module registry.

To use new Web APIs, the standard stragies for the web are available to you:

  • Use the API condionally, such that your feature still works in a lesser way without it. This is overkill for convenience functions, but might make sense for e.g. window.Intl, requestIdleCallback, etc.
  • Use an early feature test within your component and omit the feature in older browsers.
  • Include a polyfill until such time that the platform threshold is raised.

The latter will naturally happen as part of T178356 and per the above does not seem to require additional feature checks.

For convenience functions, it seems like a case of better understanding the audience for the code and writing it accordingly. This isn't different from any other unavailable Web API one might use by accident. What strategy do you use for that, and what can we do to improve it?

Krinkle changed the subtype of this task from "Bug Report" to "Task".

Waiting for T178356 is one solution yes, but I don't have much confidence that will happen any time soon. Adding the check to our grade A seemed reasonable, given these browsers without NodeList.forEach are probably not worth supporting.

Given the 3 issues which only surfaced because of JavaScript production errors, this is clearly nuanced enough to be a common mistake to make by human error, so perhaps an eslint or typescript check could be used to detect forEach on Nodelist pattern in ES5 code if we're not willing to do raise the grade A requirements.

I'm looking to port more of Vector's code to ES6, which is blocked on T299677 which will reduce this happening in our own team's code, but not eradicate the problem completely as we still need to support IE11 for some experiences there.

New rule here: https://github.com/wikimedia/eslint-plugin-mediawiki/pull/78

(As a static analyser we can only detect obvious cases where the NodeList immediately invokes an unsupported method, but if that NodeList is stored in a variable and passed around that is going to be much harder to detect)

@Jdlrobson Ignoring browser compatibility is not a sustainable approach long-term, neither does it appear reasonable or feasible to blame the platform and expect a polyfil for everything that is availalable in any browser, not in the least because some feautures can't be polyfilled and because you'd still need to know to declare the dependency.

There are a number of ways to handle this, which includes better code review and awareness of browser support within your team, as well as best-effort automation in the form of ESLint plugins such as compat/compat that we recently introduced in most repositories, and more ad-hoc ones like the ones that Ed has developed. For example, there are now automatic warnings in CI and in text editors with ESLint support when you call performance.getEntriesByName() or Intl.Collator etc from eslint-plugin-compat.

Other approaches may involve using tsc as a linter which might pick up a few more cases than ESLint can out of the box (I believe there are ESLint plugins that can do this and transparently analyze code with TSC under the hood from within ESLint).

Using accidental use of unavailalbe features as a reason for raising Grade A levels is by itself imho an unsustainable direction that I cannot in good conscious support. There are many reasons to raise Grade A levels, which are generally based on audience support, not developer support. There is nothing inherently new or difficult about iterating a node list that one could measure as a significant development cost, either. It was simply a mistake to call the wrong method, and it was subsequently found and fixed, with another patch on the way to detect it for you automatically.

I'd prefer to raise our ES6 requirements - is there any reason we wouldn't do that? - but eslint seems the way to go if we're not willing to do that! Thanks for the patch Ed.