Controversies (Deferred implementation case)

[medikoo]

@domenic such tool is great idea, but code used for tests is at some points controversial. I think it's good to discuss it further.

Deferred implementation obeys all the concepts that are tested, but when setup, it fails mosts of the tests. Reasons for that are:

Deferred doesn't create extra promise when it doesn't make sense to create one (performance reasons). Following:

promise.then()

Will just return input promise, and tests of this project assume that it should return newly created promise. I don't agree that it's necessary. As you mentioned in your document then is a mechanism for applying a transformation to a promise. If there's not transformation provided I think it's very ok to return same promise.

In Deferred promise can be rejected only with an error object. It goes also other way, promise cannot be fulfilled with an error object. In result all tests that try to reject promise with some regular value, fail.

In my point of view, rejection should be only about errors, this the reason why it's solved that way in Deferred.

However the way I've solved it, I'm not 100% on now. Thing is in JavaScript we can throw also other objects and I believe it should be matched. So with 0.7 I plan to change behavior so it adheres with throw, if value can be thrown, we should be able to use it for rejection.

Still I'm wondering: When you work in real world applications, do you reject promises with not error objects? If so what's the case? and if you do, do you also sometimes throw not error objects in synchronous functions (this should go in pair in my opinion)?

In my fork I've updated the tests, so they don't test empty then calls and do all rejects with errors, then Deferred passes 100%:

[domenic]

On point 1, sorry, it's in the spec:

This function should return a new promise

On point 2: the spec doesn't give any guidance (in fact I'd say error propagation is underspecified). But my intent was to match try/catch as you said. And yeah, just as with try/catch I never reject with non-errors.

Happy to discuss further; thanks for contributing :)

[medikoo]

@domenic

Ad. 1 If it's so, this spec is not perfect. Adhering to that doesn't have logic reasoning, but affects performance, which is bad. Definitely I wouldn't change that behavior in Deferred.

Ad. 2 I plan to change it, but still I think tests in this project should match how you would use promises in real applications (currently they're far from that). If you want to test whether you can reject promise with normal value, there should be specific test for that, and other tests shouldn't be vulnerable to not related corner cases, if they are, they give false picture.

[domenic]

On 1, let me summon @kriskowal to see if he can recall why p.then() !== p is important. But also, let's turn the tables a bit. When else does Deferred return the same promise? Is it only the no-arguments case? If so, is that really a common enough case that it's worth going against the spec for performance there?

On 2, I see what your saying, but try the synchronous analogy again. If you were writing tests for try/catch, you probably wouldn't only use errors and call non-errors a "corner case." Rather, Error instances are just things that have specific nice behavior in certain debuggers; the language spec itself is entirely agnostic to what kind of objects you end up throwing.

Besides (and this is silly I admit), I love how the whole test suite gets away with just other and sentinel and nothing else :)

[medikoo]

@domenic

Ad 1. It's only the case for resolved promises: If there's no transformation for current promise provided, then same promise is returned. So if you didn't give success callback for fulfilled or error callback for rejected then same promise is returned.

Ad 2. I would definitely use only errors, and eventually if it would be needed I would write tests for corner cases where objects that are thrown are not errors.

While it's not banned by the spec, throwing not error objects is a bad practice and usually comes out of lack of knowledge: http://www.devthought.com/2011/12/22/a-string-is-not-an-error/

I think you wouldn't spot it in any popular framework/tool.

[domenic]

I'm not really going to budge on 2, sorry; I think you are still not looking at it from the perspective of a spec-test writer. There's nothing in the ECMAScript spec or the Promises/A spec encouraging the use of Errors, so tests should use the most generic possibility. Best practices are a great thing to follow, but not to use to guide a spec-based test. In the spec's view, throwing non-errors is the norm, not a corner case.

Still mulling 1, and thanks for the further info there.

[briancavalier]

Kind of interesting that the spec says "should" rather than "must" there. Honestly, before now, I had always taken it to mean "must", but maybe that's not the case? "Should" makes me think that if a promise implementation can fulfill all the behavioral requirements and isolation guarantees of Promises/A without returning a new promise, then it's a valid implementation. That said, I haven't thought through it enough to know whether that's possible, although deferred may be the proof that it is!

[medikoo]

Such behavior wasn't in deferred from a start.
I had some performance issues when playing with quite complex asynchronous flow, after profiling it came apparent that one of the reasons is that enormous number of promises is created, and it appeared that many of them (ones created "by design") were totally obsolete and didn't bring any new value.

With v0.3 I brought performance improvements, which didn't affect any applications using this implementation, one of them is above then behavior. It's now v0.6 and I never thought of reverting it back. Technically deferred creates new promise only if there is some transformation on the way, it works very well, applications just benefit from that.

[domenic]

I hope that's the case, but can't shake the feeling there's a security (in the ocap sense) concern or scenario regarding the reuse of promises. Hoping @kriskowal or Mark Miller can clarify.

[kriskowal]

@domenic @medikoo It might be okay to reuse an unaltered promise. The only use I’ve found for .then() (no arguments) is to get rid of cancelability or progress emitters downstream, but that would probably be different. I’m going to guess that Mark Miller would approve of P.then() returning P on the assumption that cancelation is not supported on P.

[domenic]

@medikoo just FYI, I haven't forgotten about this, and am indeed leaning toward allowing p.then() === p. Will work on this during the upcoming week, after EmpireJS on Monday.

[medikoo]

@domenic thanks for info. I look forward for it.

[jkroso]

@medikoo Why are you calling then with no arguments? Whats the use case?

[medikoo]

@jkroso I'm not doing that and I'm not aware of any use case to do so.

[mzedeler]

Since then() creates a new empty promise chained to the previous one, it must be possible to get into problems with the order in which done handlers are fired if then() simply returns the original promise.

I tried working out an example using jQuery, but it's executing in the wrong order that I'd expect.

[domenic]

@mzedeler jQuery is not Promises/A compliant, so any attempts to get it to pass this test suite are a lost cause.

Besides, this repo is deprecated.

[mzedeler]

I was suspecting that the wrong order was a result of non-compliance on jQuerys part.