taylor1791 / arbiter-subpub

Matt Kruse's ArbiterJS as a node module
The Unlicense
2 stars 0 forks source link

Fails silently #1

Closed mmichelli closed 9 years ago

mmichelli commented 9 years ago

You should not fail silently, makes debugging impossible when using this module. https://github.com/taylor1791/Arbiter-SubPub-Node/blob/master/index.js#L127

taylor1791 commented 9 years ago

@matt-kruse How open is Arbiter to changing such behavior?

matt-kruse commented 9 years ago

What's the proposed alternative? The goal of eating the exception was to prevent a broken subscriber from stopping others from running. In an environment where component authors each get to publish and subscribe, one person's bad coding shouldn't break everything. Only their component should fail.

mmichelli commented 9 years ago

Yes, but logging it helps. console.warn(e);

taylor1791 commented 9 years ago

I think this is a good idea (after checking if console.warn exists). If @matt-kruse signs-off, I will make the change and publish v1.0.1.

matt-kruse commented 9 years ago

Yeah, definitely a good idea to warn, no harm there. You could also add the ability to define Arbiter.onError(), which would get called in case of an exception in publish() or elsewhere. That way the user of the module could decide what to do with errors.

taylor1791 commented 9 years ago

How about creating Arbiter.onError( err, data, msg, internal_data ) which is a jQuery-style setter and unique across all instances of Arbiter (using Arbiter.create) with the default implementation being as follows.

function( err ) {
  console.warn( err );
}
matt-kruse commented 9 years ago

Make it so, but be sure to check for existence of console.

taylor1791 commented 9 years ago

It appears that console.warn is non-standard despite working in all major browsers. Do we care about this? I say we go ahead and use it regardless of standard-track (after checking for existence of course).