Update Array.prototype.find

[qbroker]

The native Array.prototype.find method is modified if it natively exists, do not modify Array.prototype.find if it natively exists. This gives unwanted side-effects, method return is nowadays not the array index but the array element value. Array.prototype.find is in browsers since versions Chrome 45, Edge 12, Firefox 25.

[YakovL]

Hello Okido,

Thanks for creating this PR. There are some issues to be solved before merging it.

First, we shouldn't use it as a fallback, as this implementation has an argument signature different from the native implementation (item vs predicate, ?thisArg). So, if we are to remove this deprecated functionality, we should remove it for all browsers at once (otherwise, it may complicate debuging).

Second, the main problem about removing this method (like any other deprecated method) is that some old plugins may break. I've took a look at TiddlyTools and found none that would break, but we should probably do the same for some other major repositories (TODO: list here). Theoretically, it can be ok to do this even if there is some relatively popular plugin that uses the old .find, but we should either:

• suggest the author to update the plugin or
• articulate the breaking change well enough for users to get aware of it once they want to upgrade – and, ideally, provide a tool to fix the issues they face (by either reverting the overwrite or fixing the broken extensions). I've shared some of my thoughts on this here.

So I propose you to switch this to complete removing of .find overwrite and share your thoughts as well.

PS And could you change/recreate the PR so that it targets dev instead of master? Otherwise this will complicate tracking changes when releasing the next version.

[qbroker]

Hi Yakov,

Very good to remove .find completely. I propose the create a legacy plugin that holds the .find and just overwrites the native .find for any plugins that require this. Targeting PR at dev is no problem, I will change this and make a new PR.

Have a nice day, Okido

[YakovL]

Yet another alternative to consider: we can update the overwriting, but inside it, check the arguments and "route" to either the native implementation or the custom one. Something like: if first argument is a function, use the native method, otherwise use the custom one. The consistency of this approach is backed up with the fact that for the native method, passing a non-function produces an error:

[1,2,3].find()

Uncaught TypeError: undefined is not a function
    at Array.find (<anonymous>)
    at <anonymous>:1:9

[1,2,3].find({})

Uncaught TypeError: #<Object> is not a function
    at Array.find (<anonymous>)
    at <anonymous>:1:9

[YakovL]

Here's my draft for this:

Array.prototype.orig_find = Array.prototype.find
Array.prototype.find = function(itemOrPredicate, thisArg)
{
    if(itemOrPredicate instanceof Function) return Array.prototype.orig_find.apply(this, arguments);
    var i = this.indexOf(itemOrPredicate);
    return i == -1 ? null : i;
};

[qbroker]

Sorry Yakov, your message ended in my spam archive before I noticed it.

Your solution seems OK to me, would you do this in a legacy-Plugin or add it to the core?

Have a nice day, Okido

[YakovL]

Hi @qbroker, I've meant this as a fix to the core, applied it to dev: https://github.com/TiddlyWiki/TiddlyWikiClassic/commit/78ca58f53cbbd7ea7c16de801a003e2f600c41e1 (will be in 2.9.5)