forEach should chain #142

Closed
grignaak opened this Issue Mar 7, 2011 · 16 comments

Comments

Projects
None yet
10 participants
@grignaak

grignaak commented Mar 7, 2011

To support chaining, each should return the object
_([1, 2, 3]).chain()
.each(console.log)
.map(function (a) {return a*a; })
.value()

The current workaround is to use tap:
_([1, 2, 3]).chain()
.tap(function (all) { _.each(all, console.log); })
.map(function (a) {return a*a; })
.value()

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Mar 20, 2011

Owner

Nope, forEach should mirror the ECMAScript 5 signature (as should every other Underscore function, where possible) ... not have a special return value.

Owner

jashkenas commented Mar 20, 2011

Nope, forEach should mirror the ECMAScript 5 signature (as should every other Underscore function, where possible) ... not have a special return value.

@jashkenas jashkenas closed this Mar 20, 2011

@vkovalskiy

This comment has been minimized.

Show comment
Hide comment
@vkovalskiy

vkovalskiy Apr 5, 2012

Spend 2 hours time figuring why does chain functioning incorrectly.. This info should be placed in docs about .each or .chain methods.

Spend 2 hours time figuring why does chain functioning incorrectly.. This info should be placed in docs about .each or .chain methods.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 5, 2012

Collaborator

note: I was confusing _.each and _.map here.

@vkovalskiy: I think the _.chain documentation is pretty explicit:

Returns a wrapped object. Calling methods on this object will continue to return wrapped objects until value is used.

It should be obvious then that _.tap is useful for.. exactly what it says:

Invokes interceptor with the object, and then returns object. The primary purpose of this method is to "tap into" a method chain, in order to perform operations on intermediate results within the chain.

@grignaak was trying to side-effect with console.log and produce the same object again, rather than obeying the behaviour of _.each (a map operation) and collecting the results in a new list. That's exactly what _.tap is for. Nobody should be expecting that behaviour by default.

edit: additions

Collaborator

michaelficarra commented Apr 5, 2012

note: I was confusing _.each and _.map here.

@vkovalskiy: I think the _.chain documentation is pretty explicit:

Returns a wrapped object. Calling methods on this object will continue to return wrapped objects until value is used.

It should be obvious then that _.tap is useful for.. exactly what it says:

Invokes interceptor with the object, and then returns object. The primary purpose of this method is to "tap into" a method chain, in order to perform operations on intermediate results within the chain.

@grignaak was trying to side-effect with console.log and produce the same object again, rather than obeying the behaviour of _.each (a map operation) and collecting the results in a new list. That's exactly what _.tap is for. Nobody should be expecting that behaviour by default.

edit: additions

@yuchi

This comment has been minimized.

Show comment
Hide comment
@yuchi

yuchi Apr 5, 2012

Contributor
_.mixin({
  chainedEach: function ( o, i, c ) {
    _.each( o, i, c );
    return o;
  }
});
Contributor

yuchi commented Apr 5, 2012

_.mixin({
  chainedEach: function ( o, i, c ) {
    _.each( o, i, c );
    return o;
  }
});
@grignaak

This comment has been minimized.

Show comment
Hide comment
@grignaak

grignaak Apr 5, 2012

I typically get around this feature by mixing in a tapEach function that calls forEach and returns the array.

On Apr 5, 2012, at 6:51, Pier Paolo Ramonreply@reply.github.com wrote:

_.mixin({
 chainedEach: function ( o, i ) {
   _.each( o, i );
   return o;
 }
});

Reply to this email directly or view it on GitHub:
documentcloud#142 (comment)

grignaak commented Apr 5, 2012

I typically get around this feature by mixing in a tapEach function that calls forEach and returns the array.

On Apr 5, 2012, at 6:51, Pier Paolo Ramonreply@reply.github.com wrote:

_.mixin({
 chainedEach: function ( o, i ) {
   _.each( o, i );
   return o;
 }
});

Reply to this email directly or view it on GitHub:
documentcloud#142 (comment)

@vkovalskiy

This comment has been minimized.

Show comment
Hide comment
@vkovalskiy

vkovalskiy Apr 5, 2012

@michaelficarra Could you tell me why nobody should expect this behavior by default (giving they did not learn ECMA5 spec by heart)? I found it rather strange that each does not do default chaining.

At least I propose to add info to each description saing about inability to chain by default. This could save time to somebody.

@michaelficarra Could you tell me why nobody should expect this behavior by default (giving they did not learn ECMA5 spec by heart)? I found it rather strange that each does not do default chaining.

At least I propose to add info to each description saing about inability to chain by default. This could save time to somebody.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 5, 2012

Collaborator

note: I was confusing _.each and _.map here.

@vkovalskiy: _.each collects the return values of the provided function, building and returning a new list. _.chain is specified (and documented!) in such a way that the result of the chained method call is preserved for the next chained call. That's the whole idea behind _.chain. When you map a function that returns an undefined value for any input, you should expect the next function to operate on a list of undefined values. There's two fixes:

  1. The original solution, using _.tap to ignore the return value of _.each:

    _([1, 2, 3]).chain()
    .tap(function(all){ _.each(all, console.log.bind(console)); })
    .map(function(a){ return a * a; })
    .value()
    
  2. Return the input manually:

    _([1, 2, 3]).chain()
    .each(function(i){ console.log.apply(console, arguments); return i; })
    .map(function(a){ return a * a; })
    .value()
    
Collaborator

michaelficarra commented Apr 5, 2012

note: I was confusing _.each and _.map here.

@vkovalskiy: _.each collects the return values of the provided function, building and returning a new list. _.chain is specified (and documented!) in such a way that the result of the chained method call is preserved for the next chained call. That's the whole idea behind _.chain. When you map a function that returns an undefined value for any input, you should expect the next function to operate on a list of undefined values. There's two fixes:

  1. The original solution, using _.tap to ignore the return value of _.each:

    _([1, 2, 3]).chain()
    .tap(function(all){ _.each(all, console.log.bind(console)); })
    .map(function(a){ return a * a; })
    .value()
    
  2. Return the input manually:

    _([1, 2, 3]).chain()
    .each(function(i){ console.log.apply(console, arguments); return i; })
    .map(function(a){ return a * a; })
    .value()
    
@vkovalskiy

This comment has been minimized.

Show comment
Hide comment
@vkovalskiy

vkovalskiy Apr 8, 2012

@michaelficarra : thanks for explanation. I've found your second solution with returning results explicitly to be very convinient. Although I do not understand the phrase:

_.each collects the return values of the provided function, building and returning a new list.

As it was stated above that forEach does not intended to return anything.

@michaelficarra : thanks for explanation. I've found your second solution with returning results explicitly to be very convinient. Although I do not understand the phrase:

_.each collects the return values of the provided function, building and returning a new list.

As it was stated above that forEach does not intended to return anything.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 9, 2012

Collaborator

note: I was confusing _.each and _.map here.

@vkovalskiy:

As it was stated above that forEach does not intended to return anything.

Nope. each/forEach does produce a list of the return values. See the documentation.

Collaborator

michaelficarra commented Apr 9, 2012

note: I was confusing _.each and _.map here.

@vkovalskiy:

As it was stated above that forEach does not intended to return anything.

Nope. each/forEach does produce a list of the return values. See the documentation.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Apr 9, 2012

Owner

Nope. each/forEach does produce a list of the return values. See the documentation.

Huh? _.each is for side effects, and returns undefined.

Owner

jashkenas commented Apr 9, 2012

Nope. each/forEach does produce a list of the return values. See the documentation.

Huh? _.each is for side effects, and returns undefined.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 9, 2012

Collaborator

Whoops, you're right. Very sorry, I don't know what I was thinking.

Collaborator

michaelficarra commented Apr 9, 2012

Whoops, you're right. Very sorry, I don't know what I was thinking.

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Aug 26, 2012

I just lost a worthwhile on this, before I read this 'issue'.

I am surprised that the creator of coffeescript, where everything is comprehensible and returned aggressively (item for item in items), which make the language rock among others, doesn't want _.each to return something... Maybe so, at least it should go to the documentation cause its not obvious at all....

I just lost a worthwhile on this, before I read this 'issue'.

I am surprised that the creator of coffeescript, where everything is comprehensible and returned aggressively (item for item in items), which make the language rock among others, doesn't want _.each to return something... Maybe so, at least it should go to the documentation cause its not obvious at all....

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Aug 26, 2012

Contributor

@anodynos I've implemented this via _.each, _.forEach. Lemme know if that works for ya.

Contributor

jdalton commented Aug 26, 2012

@anodynos I've implemented this via _.each, _.forEach. Lemme know if that works for ya.

@barneycarroll

This comment has been minimized.

Show comment
Hide comment
@barneycarroll

barneycarroll Mar 12, 2014

This is the third time I've googled this unexpected behaviour. It's telling that the only contributions attempting to rationalise the existing behaviour have gotten it wrong.

This is the third time I've googled this unexpected behaviour. It's telling that the only contributions attempting to rationalise the existing behaviour have gotten it wrong.

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Mar 12, 2014

Collaborator

@barneycarroll _.each supports chaining in the latest release (1.6.0). :)

Collaborator

braddunbar commented Mar 12, 2014

@barneycarroll _.each supports chaining in the latest release (1.6.0). :)

@barneycarroll

This comment has been minimized.

Show comment
Hide comment
@barneycarroll

barneycarroll Mar 13, 2014

@braddunbar Excellent! Serves me right for not updating. Remove the wontfix tag? This thread googles pretty highly (more so than release notes!) when searching for 'underscore forEach return value' and this is the first mention that the feature request has been implemented.

@braddunbar Excellent! Serves me right for not updating. Remove the wontfix tag? This thread googles pretty highly (more so than release notes!) when searching for 'underscore forEach return value' and this is the first mention that the feature request has been implemented.

@akre54 akre54 removed the wontfix label Mar 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment