bugfix: groupBy transducer functionality #3233
Conversation
Fixes ramda#3232 Reverts code change in #9b5d8925 Setting an absent absent/nullish accumulator, rather than pasing an empty on to reduceBy, appears to correct the cached copy behavior causing pollution in the accumulators on subsequent invocations, as well as the "passthrough"; I believe this is due due the cached shallow-copying combined with the currying of `reduceBy`. It may be more desirable to correct the copy caching to prevent the error while still deterministically setting the accumulator, but for what it's worth, it's at least in line with traditional transducer behavior to get the initial value from the transformer, albeit not *quite* this way!
|
Yep - this is a quick fix but it probably doesn't address similar problems in other transducers. I think the PR you closed may be on track of a better solution ;). |
| acc.push(item); | ||
| return acc; | ||
| }, []))); | ||
| }, null))); |
There was a problem hiding this comment.
Mmmh I think the change introduced in 9b5d892 might have been breaking changes:
[].reduce(() => 42, null);
//=> null
// but
[].reduce(() => 42, []);
//=> []So reverting this definitely fixes the bug? Your repro case doesn't have empty lists as far as I can tell so I don't see an obvious connection here unless I'm looking at this from the wrong angle?
There was a problem hiding this comment.
The connection insofar as I saw it was that in reduceBy - not so much a problem of passing an empty list, but the fact that the list accumulator is being re-cloned for each group in a way that's memoized and holds onto the original values.
Reverting the change makes the tests pass for groupby, but only for groupBy, though, so I suspect this may be ignoring other places where the same bug is showing up, which may need further digging.
Fixes #3232
Reverts code change in 9b5d892
Setting an absent absent/nullish accumulator, rather than passing an empty on to
reduceBy,appears to correct the cached copy behavior causing pollution in the accumulators on subsequent invocations, as well as the "passthrough" behavior dumping every value in every group; I believe this is due due the cached shallow-copying combined with the currying ofreduceBy.It may be more desirable to correct the copy caching to prevent the error while still deterministically setting the accumulator, but for what it's worth, it's at least in line with traditional transducer behavior to get the initial value from the transformer, albeit not quite this way!