prefer-map incorrectly states that _.map could be used instead of _.forEach

[facboy]

For the following

(function() {
    var xs,
        other;
    _.forEach(xs, function(x, idx) {
        x.data.push(other.data[idx]);
    });
})();

I get this output:

/home/user/test.js
  4:5  error  Prefer _.map over a _.forEach with a push to an array inside  lodash/prefer-map

✖ 1 problem (1 error, 0 warnings)

I'm pretty certain this is incorrect.

[jasonkarns]

The better question is if there's a better data structure that doesn't require iteration and mutation.

[facboy]

Well, it's a different question, I don't know if it's 'better'. I agree that it's not particularly nice code, but this still looks like a bug to me. Is the rule really looking at that line and determining that perhaps a refactor of the data structures is in order? I would guess not, I would guess it's doing some relatively basic static analysis and concluding that this can be easily swapped to _.map, which is incorrect.

[captbaritone]

I agree that this looks like a bug.

[jasonkarns]

@facboy yeah, apologies. I agree it's a bug.

[mik01aj]

Yes, the prefer-map rule looks just on the method name called, and doesn't look at the object. 🐞

[ganimomer]

It does appear to be the case.
I'd nitpick and say that you could write this as:

xs = _.map(xs, (x, i) => _.assign({}, x, {data: x.data.concat([other[i]])}))

But that's not particularly nicer.
It also changes the data property instead of mutating the existing array, which is not the same thing, but then again, so does every use of map instead of forEach.

Would you suggest we refine the rule? Do you think it's necessary for it to make sure the push isn't called from any of the iteratee function parameters, or member expressions of them?

[captbaritone]

I think the only additional limitation we would need to add, would be to ensure that push is not being called on the first (value) parameter, or a member expression thereof. The second (index) param should be a string and thefore not support push The third param is the collection itself, which should infact be refactored to use _.map