Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Passing an undefined topic to the pub/sub methods should cause an Error #57

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

duereg commented May 11, 2012

Current Behavior

  • publish: No Error
  • subscribe: Error Cannot call method 'split' of undefined
  • unsubscribe: No Error

Proposed Behavior

I am proposing all three methods throw an appropriate error when passed either null or undefined

  • publish: Error You must provide a valid topic to publish to subscriptions.
  • subscribe: Error You must provide a valid topic to create a subscription.
  • unsubscribe: Error You must provide a valid topic to remove a subscription.
Contributor

scottgonzalez commented May 11, 2012

If we were going to land something like this, it should definitely whitelist on the type, not blacklist. With that being said, I'm not a fan of this type of error handling.

Contributor

duereg commented May 11, 2012

Easy enough to only allow strings but it sounds like you don't like
the approach of throwing errors on invalid input?

I would like to fix the pub/sub methods so that they all react to bad input in a uniform manner.
I was thinking input validation, but would you simply prefer that the subscribe method handles undefined more gracefully?

I would like to help but would like to see what you would accept as help before continuing.

Contributor

duereg commented May 14, 2012

I changed the input validation to whitelist on type.

Had an idea that I could check for the existence of the "slice" function on the "topic" parameter before using it, and if it doesn't exist, throwing a meaningful error there. Not exactly a huge improvement IMO, but at least would give the user a better idea what's going on.

Any of this sound appealing?

Contributor

shellscape commented May 14, 2012

not sure I agree with this pull. when you hit the gas in your car, your car doesnt check to make sure its a green light.

On one hand, I think this kind of error checking can get nit picky (to the point where you fall into a defensive programming trap), but I think that since the topic is an essential value for both publishers and subscribers to work correctly, then throwing a more helpful error when the topic is invalid should be done (IMO). I think it boils down to this:

  • Be minimalistic, but throw meaningful errors when essential values aren't the expected type/range/etc
  • Prevent consuming developers from unnecessarily seeing mystery WTF exception messages from your OSS lib. In other words, decently written app code, when consuming a good lib, should be an opportunity for the lib to say "Um, no, you're using it wrong" rather than the app dev to say "What the heck - mystery exception AGAIN from amplify....and....crap....it's a minified copy". (note that I said 'decently written' - I don't think any OSS lib should hold your hand so much it defends you from being a dummy)

That being said, I only feel this way about essential pieces like topic. The data arg(s) are solely the consuming dev's responsibility...

Contributor

scottgonzalez commented May 14, 2012

I think I can live with what @ifandelse just said. I don't think we should document the fact than an error is thrown for specific invalid conditions though.

@duereg Can you fix the whitespace in the code? The indentation is messed up and there is a lack of spacing around parens.

Following up for @duereg on the formatting request from @scottgonzalez - we typically try to follow the jQuery core guidelines, so refer here if you have questions - or feel free to ping one of us. (Jump in, Scott, if you need to redirect that. :-) )

Contributor

duereg commented May 15, 2012

Updated the new code to match the jquery style guidelines.

One easy optimization that could be made is that all three methods could throw the same error message.

e.g. "You must provide a valid topic."

Let me know if you want to save some bytes or are fine with it the way it is.

Contributor

duereg commented May 15, 2012

Also, in response to @shellscape, you are correct that my car will not prevent me to running a red light or driving off a cliff.

However, when I go to fill up the fuel tank, the gas tank's input is constrained so that it only accepts fuel from the unleaded pump, and the diesel pump won't fit.

I think that is the better analogy. You can still use amplify to do mind-numbingly stupid things if you like, but it won't start if you don't give it the proper fuel.

@scottgonzalez scottgonzalez commented on an outdated diff May 17, 2012

core/amplify.core.js
@@ -14,6 +14,10 @@ var slice = [].slice,
var amplify = global.amplify = {
publish: function( topic ) {
+ if ( typeof topic !== "string" ) {
+ throw new Error("You must provide a valid topic to publish to subscriptions.");
@scottgonzalez

scottgonzalez May 17, 2012

Contributor

spacing: Error( " (same for all three)

@scottgonzalez scottgonzalez commented on an outdated diff May 17, 2012

core/test/unit.js
@@ -1,5 +1,30 @@
module( "amplify.core pubsub" );
+test( "topic", function() {
+ expect(3);
+
+ try {
+ amplify.publish(undefined, function() {} );
+ }
+ catch (err) {
@scottgonzalez

scottgonzalez May 17, 2012

Contributor

} catch ( err ) {

@scottgonzalez scottgonzalez commented on an outdated diff May 17, 2012

core/test/unit.js
@@ -1,5 +1,30 @@
module( "amplify.core pubsub" );
+test( "topic", function() {
+ expect(3);
+
+ try {
+ amplify.publish(undefined, function() {} );
+ }
+ catch (err) {
+ ok(true, err);
@scottgonzalez

scottgonzalez May 17, 2012

Contributor

spacing and check the error message

@scottgonzalez scottgonzalez commented on an outdated diff May 17, 2012

vsdoc/amplify-vsdoc.js
@@ -88,7 +88,7 @@ amplify.subscribe = function( topic, callback ) {
/// 
	3. amplify.subscribe( topic, context, callback, priority )
/// 

API Reference: http://amplifyjs.com/api/pubsub
/// </summary>
- /// <param name="topic" type="String">Name of the message to subscribe to.</param>
+ /// <param name="topic" type="String">Name of the message to subscribe to. An error is thrown if topic is null or undefined.</param>
@scottgonzalez

scottgonzalez May 17, 2012

Contributor

Remove these changes.

Fixed spacing issue with Error message in amplify.core. Topic unit te…
…sts now check message content of error thrown. Removed amplify-vsdoc comments related to checking topic for null or undefined.
Contributor

scottgonzalez commented May 31, 2012

Thanks, landed in f3d9dd2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment