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

[duereg]

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.

[scottgonzalez]

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.

[duereg]

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.

[duereg]

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?

[shellscape]

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.

[ifandelse]

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...

[scottgonzalez]

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.

[ifandelse]

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. :-) )

[duereg]

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.

[duereg]

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]

Thanks, landed in f3d9dd2ab3363d170b7a802f6819a58cec1e4a4a.