Unhandled exception from updateStickyRows with patched Array prototype

[magnusbakken]

If you've patched the Array prototype in your project (not a great idea, I know), there are a couple of places in the project where the code accidentally includes the fields added to the prototype in iterations. This causes an exception from the updateStickyRows function in virtual-scroll/utils.ts. The code looks like this:

for (let i in rows) {
  if (stickyState[i]) {
    const row = rows[i];
    row.style[type] = `${coeff * (offset + (coeff * agg))}px`;
    agg += row.getBoundingClientRect().height; // TODO: cache this and update cache actively (size change)
    row.style.display = null;
  }
}

Since rows and stickyState are both arrays, any method added to the Array prototype will be included in the iteration, and if (stickyState[i]) will return true as well. Then it fails when you do row.style[type] = ..., since the row variable is actually some random function object.

There's a similar line in the file extracted-code-group.ts.

The easiest fix is to use for (let i = 0; i < rows.length; i++) instead.

[shlomiassaf]

@magnusbakken This does not sound right...

How did you patch the array prototype?

Did you use Object.defineProperty? Did you set the configurable and enumerable properties to false?

[magnusbakken]

I'm sorry, I should've been more explicit about what I meant by patching the prototype. No, I haven't been using Object.defineProperty. I can see that doing so solves the problem, and will rewrite my own prototype overrides to set enumerable to false. Thanks for the tip!

However I would still recommend using for (let i = 0..., since doing Array.prototype.foo = ... is a fairly common pattern, and not everybody is in a position to easily change it.

[shlomiassaf]

@magnusbakken I will wait with this for now, not sure how to tackle this because I don't think this should be an issue for 99% of the users.

If I see this come up again, I'll see how we can address the issue.

While it looks like a minor patch there are several points in mind:

• Making sure performance is not effected.
• Verifying that all places it occurs in are transformed (it might be in places you didn't use, like plugins)
• Making sure it is forbidden through a linter

I understand that while patching is not common and not recommended, it might still happen here and there, however, I believe that most prototype patches done by 3rd party libraries are done using Object.defineProperty with enumerable set to false and when it's done locally the developer can always fix it.

Btw, even if a 3rd party lib has done this the wrong way you can always fix that by overriding the property assignment with a new one using Object.defineProperty.

[magnusbakken]

No worries. I agree it's not a very important thing, and the issue actually inspired me to simply get rid of all the array prototype patching in my own project, so some good came of this.