Bluebird support

[TimBeyer]

trafficstars

Just as an FYI, we're in the process of migrating from Q to bluebird so a new shim was needed. I did a quick port from cls-q, only adapting the tests slightly. It can be found at https://github.com/TimBeyer/cls-bluebird

[othiym23]

Thanks, Tim! Do you need this just for Redis? I'm considering maybe folding these shims into cls-redis and applying them automatically (and maybe eventually pulling that up into CLS itself). I don't really like how you have to apply this stack of shims to get CLS working properly with average applications.

[TimBeyer]

Yes, this shim was needed in redis related parts of the application.
I'm not a 100% sure though if the problems couldn't also appear somewhere else.

Probably it's a good idea to apply these shims automatically as part of CLS, but how exactly would you know what libraries to patch? If you use some kind of explicit configuration that tells you what libraries are in use, how would the user know which ones are actually relevant to mention?

And if you parse the package.json to find the relevant modules, you'll have to do the whole thing for every dependency recursively. You could also shim require to automatically apply shims when the relevant libraries are required, but that feels a bit intrusive.

So how would you approach this?

[othiym23]

Probably by patching the module loader, and you're right about it being intrusive. However, short of turning CLS into a source-to-source transformer like browserify or streamline, it's the only way to do it seamlessly. It's the approach we use in node-newrelic, and so far that hasn't bitten us (except for convincing people they really do need to load our module before they do anything else). Of course, CLS exists because New Relic needed it, so I'd need to figure out how to support both modules having their own hooks into module loading, but that shouldn't be too tough.

Right now users have to know where CLS's edges are, and either add these shims or do some explicit binding where those edges are. It's not that cumbersome, but it could be cleaner. What do you think? @phated and @dazld, do you have any opinions on this?

Either way, I'll either point at this module or fork it soon, because the asyncListener API has changed on master and CLS needs to update to match. Thanks for putting this together!

[phated]

For all of the patches, couldn't you do:

try {
  var patchRedis = require('cls-redis');
  var redis = require('redis');
  patchRedis(redis);
} catch(){}

[dazld]

As the shims are all quite small / repetitive, and seem to be focused on interaction with redis, pulling them into the cls-redis repo seems like a great idea, actually. Synchronisation of all of the diverse libraries (even if you only, as an enduser, probably use a couple) is a pain. Much easier to make PRs in one place to support new shims in the future, and have those shims pushed out in sync with the main cls version.

Single try catch pass (as @phated mentions) on load, maybe behind an option, on by default, and it seems good to me. Document it on the library page, and people can see what is going on.

tl,dr: I'd support having all of these in cls-redis too, until there's a solution that doesn't need shimming. I don't think it should be in the main library, as not everyone is using redis etc, and patching require seems like a step too far for a reusable library.

What do you think @TimBeyer ?

[dazld]

@othiym23 should we prep up a PR to cls-redis with all of these shims included..?

[othiym23]

Thanks for your incredibly helpful feedback, Blaine and Dan! It should result in a much stronger next version of CLS!

@dazld There are other use cases where the promises shims are necessary (other database layers, mostly), so I think the right thing to do is to use the pattern suggested by @phated, but pull all of this up into CLS proper. I need to update CLS anyway to deal with changes made to the asyncListener API, which I'll be working on in early March.

If you want to play around with the pattern before then and end up with a PR, I'm sure it would be helpful. Especially with more tests! But don't feel any pressure.

[nmccready]

@TimBeyer how should one use this in express?

[RobinQu]

Hi~

It seems the problem with bluebird cannot be solved at all. For instance, a projects relies on multiple versions of bluebird because of its nested dependencies. npm3 won't remove these "unresolvable conflicts" neither.

So the result is that even if I patch bluebird in the entrance of my application, an arbitrary version of bluebird on the promise chain will render the patch useless.

And I assume patching all the bluebird in the subtree of node_modules would be feasible, but tricky as well.

With the advent of node@6, async/await along with native Promise adopted in early versions, bluebird will fade out in future.