fix($http): throw error if success and error methods do not receive a fu...

[petebacondarwin]

...nction

Closes #11330

[petebacondarwin]

@IgorMinar - can you review and merge this if you feel it appropriate?

[pkozlowski-opensource]

This sound like a sensible approach to me but we don't do much of this kind of defensive programming in other places. Anyway, if it saves people time, why not merging it.

[pkozlowski-opensource]

The patch itself looks good to me.

[jbedard]

The other option would be adding fn && in front of the fn wrapper methods. Then success/error are more like the standard promise methods when it comes to null/undefined (I don't know what that behavior is, but I think consistency is good...).

[pkozlowski-opensource]

If we want to stick to the A+ spec then we should ignore non-function callbacks: https://promisesaplus.com/#point-23

[petebacondarwin]

Hold on! This is about the custom error and success method nothing to do with the A+ promise spec.

The complaint is that the developer error of not defining the function before using it is currently difficult to debug. Ignoring such errors will make it even more difficult to debug what is going on. At least, right now, we get an error telling us that there is "something" wrong but that the error message is too vague.

[pkozlowski-opensource]

@petebacondarwin I know, I've brought promises because of this comment

[IgorMinar]

This pr looks good to me. I can't imagine that the spec authors would want what looks like a programing error to be intentionally silenced.

Since the CI is green I assume that aplus spec passes, in which case I'd say let's merge it in.

I would however make a note of of the changed behavior in the commit (and maybe even consider this as a breaking change) since people that made errors in the past will now get unexpected exceptions after the upgrade. Even if we consider this to be a breaking change, I think we should merge it into the stable branch.

[petebacondarwin]

error() and success() have nothing to do with the A+ spec. They are our own bespoke methods, so we can do what we like. Happy to add a BREAKING CHANGE message, although if this breaks someone's app then it was broken already.

[petebacondarwin]

Actually, there is no breaking change. The exception would still occur before, it is just that the error message was more vague as it happened asynchronously.

[toxaq]

Has this changed it so you have to define both a success and error function? Upgrading from 1.3.15 breaks my previously unbroken app for everywhere I chose not to handle the error method. It's also not easy to debug as I'm not getting any stacktrace outside of the angular library.

[petebacondarwin]

@toxaq no - this only affects the success(fn) and the error(fn) methods. These two methods require a callback to be passed. This commit just throws an error straight away if you do not provide the fn parameter.

[toxaq]

@petebacondarwin you're right, I just realised it's a byproduct of the way I've written my services. Sorry for the bother.