Bacon.mergeAll() doesn't check for undefined arguments

BlackYoup commented 7 years ago

Hi,

I just had a problem with Bacon.mergeAll() returning a cryptic error when one of its arguments is undefined.

Example:

const Bacon = require("baconjs");
Bacon.mergeAll(undefined, Bacon.once("yolo")).log();

Output:

TypeError: Cannot read property 'dispatcher' of undefined
    at /tmp/node_modules/baconjs/dist/Bacon.js:3122:21
    at CompositeUnsubscribe.add (/tmp/node_modules/baconjs/dist/Bacon.js:1141:13)
    at new CompositeUnsubscribe (/tmp/node_modules/baconjs/dist/Bacon.js:1119:10)
    at Dispatcher._subscribe (/tmp/node_modules/baconjs/dist/Bacon.js:3141:14)
    at Dispatcher.subscribe (/tmp/node_modules/baconjs/dist/Bacon.js:1290:28)
    at Dispatcher.subscribe (/tmp/node_modules/baconjs/dist/Bacon.js:242:17)
    at Object.wrappedSubscribe (/tmp/node_modules/baconjs/dist/Bacon.js:460:30)
    at EventStream.subscribe (/tmp/node_modules/baconjs/dist/Bacon.js:1037:26)
    at EventStream.Bacon.Observable.log (/tmp/node_modules/baconjs/dist/Bacon.js:3101:8)
    at repl:1:47

There is no mention of the mergeAll call in the stack trace, so it might be hard to understand where the error comes from without reading the source code. This is with the last version of Bacon (and even a few before that one).

I would be happy to open a PR for this, but I'm not really sure on how to do such fix right now.

Thank you :-)

lautis commented 7 years ago

The same happens when any non-observable value is given as an argument for Bacon.mergeAll. An assertion there could be appropriate that every item in streams extracted from arguments is an observable.

You're not seeing mergeAll in the stack trace as the event stream is initialised when there is a subscription. The topmost line in the stack trace (Bacon.js:3122:21) is within Bacon.mergeAll source.

BlackYoup commented 7 years ago

The same happens when any non-observable value is given as an argument for Bacon.mergeAll.

Yeah, I saw that right after the report. Will update the title

An assertion there could be appropriate that every item in streams extracted from arguments is an observable.

An assertion would be useful, not sure how much overhead this implies though

You're not seeing mergeAll in the stack trace as the event stream is initialised when there is a subscription. The topmost line in the stack trace (Bacon.js:3122:21) is within Bacon.mergeAll source.

Yep, that's mostly why I opened the issue, there is no reference of the called function that triggers the error.

lautis commented 7 years ago

There is some assertions in the codebase. They cause overhead, but that hasn't been a big problem as you can use the noAssert build when needed. assemble.js strips out all function calls to functions that are named assert*.

There isn't an assertion that would run on arrays, only single items. It might be needed to create one to get also the loop removed from the build.

baconjs / bacon.js

Bacon.mergeAll() doesn't check for undefined arguments #687