Es6 style promises #531
Es6 style promises #531
Conversation
Angular 1.3.x introduced ES6 style promises. These often provide much cleaner / more concise code. This commit provides a polyfill for that functionality that will use angulars implementation if available.
There is more code that could be refactored to use it as well. Waiting to see what the community thinks before proceeding.
| return this._utils.promise(function(resolve,reject){ | ||
| function callback(authData) { | ||
| if (authData !== null) { | ||
| resolve(authData); |
There was a problem hiding this comment.
I have found it to be good practice to add a return statement before every call to resolve() or reject() to make sure no code gets run after them. I've had too many bugs where I didn't do this and they are a huge pain to track down.
There was a problem hiding this comment.
Not sure I agree with your premise. Angular promises are now A++ compliant. Part of that spec states that once a promise is resolved or rejected, it's state is fixed permanently - future calls to resolve or reject will not change the promises state. Once you call resolve(A), you can call resolve(B) or reject(E) as often as you like - the promise will always be resolved with A.
Are you possibly thinking about always returning a value from a promise handler? That makes a lot of sense. It allows chaining.
That said - I am totally willing to change it.
There was a problem hiding this comment.
No, I have more pedantic reasons for wanting the return in there. I'm not worried about the promise getting resolved or rejected multiple times since, as you said, the A++ spec prevents that. But I do think in most cases it is unexpected and unnecessary to run more code after the promise is resolved or rejected. Putting an explicit return in there will stop future lines in that function from running, which is what we want in most cases. Not doing so can lead to some nasty bugs where code runs when don't expect it to.
But yes, allowing for chaining is also a positive, so let's do it! But when you do add the return statements, you'll need to move the ref.offAuth(callback) above the if block or else the return statements will exit early causing that code to not run.
There was a problem hiding this comment.
Chaining isn't at issue here (we aren't in then callback).
I made the changes requested. IntelliJ complains that the return statements are unnecessary, but the linter passes.
|
|
||
| if (authData !== null) { | ||
| resolve(authData); | ||
| return; |
There was a problem hiding this comment.
Hmmm, not quite what I was envisioning. I'd just do return resolve(authData) and return reject("AUTH_REQUIRED"), etc. No need for it to be on a separate line. And then you can enable the chaining as you mentioned.
There was a problem hiding this comment.
I think you might be conflating these methods with $q.resolve and $q.reject, which return promises. These methods behave like deferred.resolve and deferred.reject which do not have specified return values. The original code provides chain-able promises.
See the $q constructor documentation, they aren't returning there at all. I would think it's bad practice to return the result of a function whose return value is NOT(edit) guaranteed by documentation / specification (they may be leaving it's return value unspecified intentionally to allow for future changes).
There was a problem hiding this comment.
Ahh okay, okay, I definitely mis-spoke there and you are correct about the return value/chaining thing. I still prefer stylistically to have the return on the same line as the resolve / reject (and this is actually what we do with a lot of our backend Node.js code), but I'm not going to block this PR any further on that.
|
Few more comments then this is good to go 👍 |
|
Okay, one more |
|
OK, One more argument and then I'll cave. If poking around the source I see Tell me to inline them and I'll do it. Just picking nits. |
|
Haha, no I'll let it slide this time just for you! I completely see where you are coming from and don't entirely disagree. I can't promise that I won't change it myself in the future though :) You are definitely missing one |
|
done |
Angular 1.3.x introduced ES6 style promises. These often provide much cleaner / more concise code.
This commit provides a polyfill for that functionality that will use angulars implementation if available.
The second commit refactors some code to use the new promise style.