Fix Array.prototype overwrites breaking for...in loops

[johannschopplich]

This PR …

… wishes the team a happy new year! While fixing a bug.

I'm developing a new Kirby Panel plugin, which uses a third-party JavaScript library. This library itself uses the for...in loop. The Panel itself adds two methods to the Array.prototype: sortBy and split, leading to the following error in my third-party library:

The Array.prototype contains unexpected enumerable properties: sortBy, split; thus breaking e.g. for...in iteration of Arrays.", name: "UnknownErrorException", details: "Error: The Array.prototype contains unexpected enumerable properties: sortBy, split; thus breaking e.g. for...in iteration of Arrays."

Modifying Array.prototype or any built-in prototype is generally considered a bad practice, but who cares. 🙂 Unfortunately, I can't change the third-party library and need a fix.

When you add properties to Array.prototype, they become enumerable, which means they will show up in for...in loops that iterate over arrays. This can lead to errors and bugs, especially if other parts of the codebase are not expecting these additional properties. This isn't relevant for the Panel, but Panel plugins running in the same thread.

Since the Panel has to add methods to Array.prototype and we want to avoid this error, we have another option:

• Use Object.defineProperty: This way, they won't appear in for...in loops.

My plugin depends on the third-party library, so development is kinda blocked at the moment. If I may have a wish for the new year, it's the merge of this PR. Thanks in advance!

Fixes

Any JavaScript code that uses for...in under the hood.

Breaking changes

None.

Ready?

• Unit tests for fixed bug/feature
• In-code documentation (wherever needed)
• Tests and checks all pass

For review team

• Add changes to release notes draft in Notion
• Add to website docs release checklist (if needed)

[bastianallgeier]

That's one of those decisions that we need to undo at some point. Your solution seems great to me. @distantnative what do you think?

[distantnative]

Wondering if we should already use this moment t move these to normal helper functions, then use object.defineProperty as suggested to add them in a usable way to the Array object, but mark that already as deprecated.

[johannschopplich]

I thought so as well – why not make them importable utils like all the other helpers (debounce etc.).

The base branch was changed.

[distantnative]

@johannschopplich

Do you have an idea why this doesn't seem to work?

export function split() {
//...
}

Object.defineProperty(Array.prototype, "split", {
	value: split,
	enumerable: false, // This makes sure the property is not enumerable
	writable: true,
	configurable: true
});

[johannschopplich]

Mmh, I guess because the this has different context. In the standalone function, it doesn't has the properties of Array.protoype. 🤔

[distantnative]

Yes, the first parameter must be the array then. But somehow still didn't work for me

[distantnative]

Ok I just had a stupid typo somewhere that completely threw me off.

What do you think of 48fe180?
Is Array.fromObject a similar issue?

[johannschopplich]

48fe180 looks fine. Great solution while being backwards-compatible. 👏

Regarding Array.fromObject: Somehow it doesn't seem to throw a JS error, but I think migrating it as well would be a sustainable choice. Especially since it is rarely used.

Edit: Feel free to close this PR in favor of your commit.

[distantnative]

Closing in favor of #6140