Incorrect _.bind when (ab?)using _.find with property shorthand and a thisArg

[davidshepherd7]

In 3.10 it was possible to use a three argument shorthand for find, such that this

_.find(collection, 'id', x);

is equivalent in 4.13 to

_.find(collection, ['id', x]);

However the codemod outputs

_.find(collection, _.bind('id', x));

which fails. This "feature" doesn't seem to have been documented anywhere, so I think it's an unintended consequence of combining thisArg and _.matchesProperty. I suspect the same thing worked for all functions that could take a property shorthand and a thisArg.

But anyway it somehow it got used in a few places in our code base, so I thought you might like to know that the codemod doesn't deal with it. Maybe you could add a note in the readme, or even implement the migration?

[jfmengels]

Thanks for reporting this!

Hmm... I'm not really sure what to do here. I think in quite some cases, you won't be able to know for sure whether the code is supposed to bind to this or whether it's the shorthand you mention. If the second argument is a function expression or a string, we'll know what to do, but if it's a variable, I don't know if it's possible with jscodeshift. And I'm worried that in too may cases, it will be a variable, and being on the safe side by not applying the change and printing a warning would defeat the purpose of this tool as it moves a lot of the work back to the user.

Proposition: I'll add an exception at least when we see that the second argument is a string for find, and I'll add a note in the README.md, but I'll keep applying the change when dealing with variables.

What do you think?

@jdalton Do you know of other functions that were like this in v3? I'm thinking of filter, reject for instance.

[jdalton]

Do you know of other functions that were like this in v3? I'm thinking of filter, reject for instance.

Any of the ones that processed their iteratees with _.iteratee (or the v3 equiv).

[davidshepherd7]

Yeah in my opinion just handling the case where it's a hard coded string and adding a note in the docs is enough. To handle all cases perfectly you would need to evaluate arbitrary code until you find out whether the result is a string or a function, and that sounds very difficult!

Hopefully it's not something that was widely used since it's undocumented!

[eburi]

I got good results using this in lib/this-arg-removal.js:

  .forEach(function (p) {
    var thisPos = methodsThisPos[getName(p)];
    var args = p.value.arguments;
    if (args.length < thisPos) {
      return;
    }

    if(args[thisPos - 2].type == 'Literal') {
      p.value.arguments = args.slice(0, thisPos - 2).concat([
          j.arrayExpression([args[thisPos - 2], args[thisPos - 1]])
      ]);
    }
    else {
      p.value.arguments = args.slice(0, thisPos - 2).concat([
        j.callExpression(
            j.memberExpression(j.identifier('_'), j.identifier('bind')),
            [args[thisPos - 2], args[thisPos - 1]]
        )
      ]);
    }
  });