praekeltfoundation / vumi-jssandbox-toolkit

Vumi JavaScript Sandbox toolkit.
BSD 3-Clause "New" or "Revised" License
7 stars 8 forks source link

Fix use of `join` inside chained lodash sequences. #230

Closed hodgestar closed 8 years ago

hodgestar commented 8 years ago

Lodash only gained a _.join(...) function in Lodash 4.0.0 and we're relying on 2.4.x. That means that sequences like:

_.chain(["a", "b", "c"])
   .join("\n")

relies on the built-in join understanding chained objects, which seems to fail in some obscure circumstances I don't fully understand yet.

peterp commented 8 years ago

I think because _.chain() returns an lodash sequence, so you need to convert to an array with .value(), eg: _.chain(["a", "b", "c"]).value().join("\n")

hodgestar commented 8 years ago

This is the code that was failing: https://github.com/praekelt/vumi-jssandbox-toolkit/blob/master/lib/states/paginated.js#L140-L146.

Moving the .value() to after the .concat() fixed it.

justinvdm commented 8 years ago

I would have also expected it to be a case where we didn't unwrap a lodash sequence, but in the code example, we are unwrapping with .value() (.join() can happen before .value(), since lodash wraps Array.prototype.join).

peterp commented 8 years ago

@justinvdm I'm not sure that they do support it in 2.4.* https://github.com/lodash/lodash/blob/2.4.2/doc/README.md, just based off a search in the documentation.

@hodgestar if you swop around .value() and .join() maybe that'll work?

justinvdm commented 8 years ago

@peterp The docs for 2.4.2 say so, so I'm wondering if there is something else going wrong here:

In addition to Lo-Dash methods, wrappers also have the following Array methods: concat, join, pop, push, reverse, shift, slice, sort, splice, and unshift

peterp commented 8 years ago

@justinvdm Aha, there's an additional footnote below the section of the docs that you've quoted.

The non-chainable wrapper functions are: .., join, ...

justinvdm commented 8 years ago

@peterp I think there they are talking about _.join, which probably works subtly differently to Array.prototype.join. Otherwise, their docs are contradictory (the latest docs also mention both of the notes we referenced here).

2.4 does seem to work with .value(), so I think this sounds like a lodash bug.

> let u = require('lodash');
undefined
> u.VERSION
'2.4.0'
> u.chain(['a', 'b', 'c']).map(v => v + v).join('\n').value()
'aa\nbb\ncc'
justinvdm commented 8 years ago

@peterp @hodgestar Ok, so I looked into this a little more, wanted to figure out those two parts in the docs that seemed to contradict each other.

@peterp I think there they are talking about _.join, which probably works subtly differently to Array.prototype.join. Otherwise, their docs are contradictory (the latest docs also mention both of the notes we referenced here).

I was wrong here, lodash chains always use _.join, not Array.prototype.join.

It seems we aren't the only ones who got confused by those two seemingly contradictory parts in the docs: https://github.com/lodash/lodash/issues/2166.

By non-chainable wrapper functions, they mean functions that won't work with implicit chaining but will work with explicit chaining:

$ node
> let u = require('lodash')
undefined
> u.VERSION
'2.4.2'
> u.chain(['a', 'b', 'c']).join(',').map(v => v + v).value()
[ 'aa', ',,', 'bb', ',,', 'cc' ]
> u(['a', 'b', 'c']).join(',').map(v => v + v).value()
TypeError: u(...).join(...).map is not a function
    at repl:1:30
    at REPLServer.defaultEval (repl.js:252:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:417:12)
    at emitOne (events.js:95:20)
    at REPLServer.emit (events.js:182:7)
    at REPLServer.Interface._onLine (readline.js:211:10)
    at REPLServer.Interface._line (readline.js:550:8)
    at REPLServer.Interface._ttyWrite (readline.js:827:14)
hodgestar commented 8 years ago

I'm closing this for the moment -- we can re-open if we get a clearer / more reproducible picture of the circumstances under which things will fail.

justinvdm commented 8 years ago

Ok reproduced the problem on node 5.9.1. Quite randomly, grunt-legacy-log is the culprit of our .join problems. It mixes in underscore.string, so _.join becomes something completely different that doesn't work on arrays the way we would expect it to.

This problem would only happen in tests, not production, since grunt-legacy-log wouldn't be around to do its mixin hackery in production.

We can work around this in the toolkit itself, or work around it in each project using the toolkit like so.

hodgestar commented 8 years ago

Awesome sleuthing!