Implement mappingHint for wrapCallback, fixes #246

[apaleslimghost]

I definitely think this is a useful thing to have (seems like an omission that it was only for EventEmitters and not wrapCallback too). Can someone check if the docs make sense to someone who isn't sleep-deprived?

[apaleslimghost]

From the other issue thread:

We usually avoid optional parameters, because they don't work with currying, but since this is a constructor function, not a stream method, that doesn't apply here (unless anyone disagrees!)

Thoughts?

[svozza]

That is a far better explanation of what mapping hint does than we have in the Stream Objects section of the docs, it might be worth updating there too. One thing I'd be worried about though is that we now have divergent behaviour for streamifyAll and wrapCallback: that could potentially be very confusing for users.

As for the optional arguments, normally I think they should be avoided as much as possible but given that currying is not an issue here there's probably an argument to be made for making an exception this time.

[vqvu]

I dunno about this...is it typical for node-style callback functions to pass in multiple values? I'm not sure the divergent behavior with streamifyAll is worth it.

The mapping hint thing was originally for event emitters.

[apaleslimghost]

Off the top of my head, request has multiple arguments (err, res, body). There are almost certainly more. @Bolus, what was your use case?

It seems to me that you won't necessarily want to use the same mapping hint for every function in an object you're streamifying, so I'm not sure mappingHint makes sense for streamifyAll?

---

p.s. Is there a particular reason streamifyAll doesn't use wrapCallback internally, but has its own wrapWithContext?

[d4h0]

@quarterto: I was trying to use 'request'.

In the past I had problems with 'fs.exists' which has only one callback argument (no error argument, only a Boolean if the path exists or not).

Btw., how else would I use a function like 'request' without 'wrapCallback'?

'consume' is the only solution I could find but this would be much more complex than just using 'wrapCallback'

[apaleslimghost]

In request's case, it's actually a readable stream, so you could just do _(request(...)).

On fs.exists, from the Node docs:

fs.exists() is an anachronism and exists only for historical reasons. There should almost never be a reason to use it in your own code.

In particular, checking if a file exists before opening it is an anti-pattern that leaves you vulnerable to race conditions: another process may remove the file between the calls to fs.exists() and fs.open(). Just open the file and handle the error when it's not there.

fs.exists() will be deprecated.

[svozza]

streamifyAll all uses a function that binds the context because you could be streamifying packages that have methods reliant on that context. You can see the initial rationale in #221.

[apaleslimghost]

I was just wondering why that couldn't be put in wrapCallback. I can't think when keeping the context of the function would be a bad idea?

[svozza]

Yeah, good point, I think that makes perfect sense.

[apaleslimghost]

Cool, I'll open a separate pull request for that.

Getting back on track: I don't think I'd expect streamifyAll to have something like mappingHint, as you'd almost always want different hints for different functions. So I don't really think it would be inconsistent to implement mappingHint for wrapCallback, and we gain a lot by implementing it.

[vqvu]

Fair enough. No objections here from me then.

Agreed that mappingHint doesn't make sense with streamifyAll.

Regarding the default behavior, I think we should push an array if there are more than one success argument. You mentioned that it'd be weird if a callback was called with different arguments at different times, but that's more of a user problem, imo (they can provide a mappingHint if this is the case). Plus, wrapCallback only allows a single callback anyway, since it pushes nil afterwards. Bluebird#promisify does this too.

The benefit of this being that streamifyAll will also work better with functions that pass in multiple success arguments once #248 is merged.

[vqvu]

Like @svozza said, maybe move this to the Stream Objects section and put a link to that section from here?

[svozza]

Yeah, as long as we document any gotchas between the two functions' behaviour I think we should go ahead with the PR.

[apaleslimghost]

Now that I think about it, this PR is a breaking change. It's possible that someone is relying on the behaviour of wrapCallback only emitting the first result and discarding the rest. If we implement this they'll suddenly start getting unexpected arrays in their streams.

Given that, I think we should wait until 3.0 for this.

Collecting the todos here for future reference:

• change hintMapper to return arrays in the default mapper if more than one argument is passed
• move mappingHint documentation to Stream Objects section, link there from wrapCallback

Anything I've missed?

[svozza]

I still think you can put a version of the mappingHint documentation into the current branch's Stream Object section because it's far better than what's already there now.

[apaleslimghost]

Good idea, I'll do that tonight.

[vqvu]

Can't we add mappingHint but without the array behavior for the default mapper along with a note that it will change in 3.0? That way people have a temporary, if not complete, solution.

[apaleslimghost]

Sure, if we're happy for it to go out without that?

[vqvu]

It wouldn't be any worse than what we have right now.

[apaleslimghost]

@vqvu was that all it needed?

[vqvu]

One more test for wrapCallback with the default mapper and passing multiple values.

[vqvu]

Looks good!