Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compliance branding (e.g. then.aplus) #2

Closed
domenic opened this issue Oct 24, 2012 · 25 comments
Closed

Compliance branding (e.g. then.aplus) #2

domenic opened this issue Oct 24, 2012 · 25 comments

Comments

@domenic
Copy link
Member

domenic commented Oct 24, 2012

From #1:

.then.aplus is actually ok with me. However, there's nothing to prevent someone from just slapping a truthy .aplus on their busted .then in a similar way that $.Deferred chose to use .then--i.e. without fully recognizing the implications. Maybe now that we have the Promises Test Suite, we can codify a Promises/A+ set of tests, and the community will have a way of ensuring that an implementation actually deserves .aplus?

However, I'm torn on whether we should require .aplus in order to consider something a promise at all. Maybe .then should signify a "promise-like thenable", and .aplus simply tightens that and allows you to make even more assumptions about the thing's behavior? Or maybe that's just asking for more pain and special casing? Thoughts?

@domenic
Copy link
Member Author

domenic commented Oct 24, 2012

I think a test suite would be crucial to such branding, and will happily volunteer to expand or augment the existing one. Then the problem reduces not to confused misunderstandings, but to maliciousness, which we shouldn't try to guard against.

I'm not sure what "require .aplus in order to consider something a promise at all" means. For what purpose? For me, the use case would be as described in my previous comment: for reusable promise-accepting libraries that should be implementation agnostic, but need to be sure that they're really getting promises that they can compose.

So the spec would perhaps say "implementations which meet all of these requirements are encouraged to add a truthy aplus property to their then functions, so that third parties can consume such promises with confidence in their behavior."

The "thenables" concept still has value, in that you can assimilate even jQuery "promises" into well-behaved ones. In other words, there's no need to check for aplus when assimilating. It's more for the case when you're purposefully not assimilating, i.e. for reusable functions (like those in Chai as Promised) that apply to any compliant promise implementation.

@briancavalier
Copy link
Member

Re: expanding the existing test suite. Yes, crucial, agree. I would gladly help as well.

Sorry for my confusing wording. I was getting at what you said in your last two paragraphs, and so I agree :)

@wycats
Copy link

wycats commented Oct 24, 2012

I have several framework use-cases where I need to detect whether an app-provided value is a promise or not, and having a canonical way to do this would be helpful.

Additionally, this would help .resolve(promise) have less error-prone semantics as well.

@briancavalier
Copy link
Member

Beyond the typical Promises/A test ("is it a thenable or not"), do you feel like testing specifically for .then.aplus would help in your use cases?

Could you expand on how it helps the semantics of .resolve(promise)?

I feel like .aplus certainly doesn't hurt anything, and is a nice stamp of approval. Just trying to get a feel for whether it has any other uses beyond what @domenic outlined (choosing not to assimilate).

@wycats
Copy link

wycats commented Oct 25, 2012

@briancavalier In Ember, we check whether a deserialized URL segment is a promise in order to decide whether to pause application routing until the promise is resolved. (in the meantime, we move the application into a conventionally named loading state).

I would like a good way to detect promises with chainable .then semantics. I'm not even sure that I would attempt to assimilate "broken" promises, instead asking users to do RSVP.when(jQueryPromise). I am worried about false positives, causing weird behavior or "forever loading".

@lsmith
Copy link
Contributor

lsmith commented Oct 25, 2012

FWIW, I'm against .aplus branding. Spec compliance isn't a boolean property, as the spec may change over time. The core of the spec is the definition of the behavior of promise.then(fulfilled, broken), so duck typing thing.then as a function should suffice. It's imperfect, yes, but it's not API noise like .aplus.

I would rather the merits of spec compliance be the propagating factor for A+, and having an associated test suite goes a long way to making this more convenient for implementers.

@lsmith
Copy link
Contributor

lsmith commented Oct 25, 2012

Aaaaand now I notice that there's a whole lot of (excellent) discussion that happened on the gist that I hadn't read. Nothing that changed my mind on .aplus, though.

@wycats
Copy link

wycats commented Oct 25, 2012

@lsmith compliance may not be a boolean property, but "I want to be a promise" pretty much is. I'm more worried about false positives than a guarantee of spec compliance.

@briancavalier
Copy link
Member

After sleeping on it and reading the latest round of comments, it seems like we mostly (or perhaps completely?) agree that a test suite is much more important than an .aplus property. So, I'm basically prepared to move forward, for now, without mentioning .aplus in the proposal, and pointing to the test suite.

My only lingering concern is that there are already "promises" in the wild, e.g. $.Deferred, for which the duck-type test passes, but most likely will never become Promises/A+ compliant. Should we worry/care about that, and if so, is there any way we can address it other than continuing to rely on assimilators like when() and Q.when()? As an aside, maybe trying to convince them to deprecate $.Deferred.then (as @domenic has said on other forums) is the right thing?

@domenic
Copy link
Member Author

domenic commented Oct 25, 2012

My only lingering concern is that there are already "promises" in the wild, e.g. $.Deferred, for which the duck-type test passes, but most likely will never become Promises/A+ compliant.

This is precisely the problem. I suppose we could just maintain a blacklist (e.g. "if it has a pipe property in addition to then, it's no good"), but I was hoping for a cleaner solution with compliance-branding a la define.amd.

@lsmith
Copy link
Contributor

lsmith commented Oct 25, 2012

How is $.Deferred not compliant?

@wycats
Copy link

wycats commented Oct 25, 2012

If $.Deferred is compliant, it defeats the entire purpose of this endeavor.

In particular, jQuery-produced promises don't support chained .then (they use a non-standard .pipe) and their .pipe method doesn't propagate errors. Instead, jQuery bubbles out exceptions instead of catching and propagating them.

@domenic
Copy link
Member Author

domenic commented Oct 25, 2012

How is $.Deferred not compliant?

It fails the following test suite tests:

  [Promises/A] Chaining off of a fulfilled promise
    when the first fulfillment callback throws an error
      1) should call the second rejection callback with that error as the reason

  [Promises/A] Chaining off of a rejected promise
    when the first rejection callback returns a new value
      2) should call the second fulfillment callback with that new value
    when the first rejection callback throws a new reason
      3) should call the second rejection callback with that new reason

  [Promises/A] Chaining off of an eventually-fulfilled promise
    when the first fulfillment callback throws an error
      4) should call the second rejection callback with that error as the reason

  [Promises/A] Chaining off of an eventually-rejected promise
    when the first rejection callback returns a new value
      5) should call the second fulfillment callback with that new value
    when the first rejection callback throws a new reason
      6) should call the second rejection callback with that new reason

  [Promises/A] Multiple handlers
    when there are multiple fulfillment handlers for a fulfilled promise
      7) should call them all, in order, with the same fulfillment value
    when there are multiple rejection handlers for a rejected promise
      8) should call them all, in order, with the same rejection reason

  [Extension] Promises are always resolved asynchronously
    9) should be asynchronous for already-fulfilled promises
    10) should be asynchronous for already-rejected promises
    11) should be asynchronous for eventually-fulfilled promises
    12) should be asynchronous for eventually-rejected promises

jQuery-produced promises don't support chained .then

As of 1.8, their then and pipe are identical.

@briancavalier
Copy link
Member

@wycats, not sure what you meant by "the whole endeavor", so forgive if I'm reading too much into it. Even if $.Deferred was compliant with Promises/A+, I think there is still value. For one, the handlers-that-return-promises behavior is not specified in Promises/A, so I think that's valuable to codify. And for people who will write their own promise impl, and it seems like there are people writing new ones every day, it will be helpful.

@lsmith
Copy link
Contributor

lsmith commented Oct 25, 2012

Thanks for indulging me, and yay for test suites! So they don't treat errors as triggers for broken and they notify synchronously.

Is it unlikely that they could fix the former, considering it a bug? The latter seems like a harder issue, but worth pressing on.

@briancavalier
Copy link
Member

I think there has been talk of making $.Deferred async, but I don't know what the current status is. So, maybe we can make some headway there.

As for error propagation/handling and exception translation, I don't have as much hope. I've been told flat out by jQuery committers that they will not support that aspect of Promises/A. It's possible that could change, of course, but probably not anytime soon.

@wycats
Copy link

wycats commented Oct 25, 2012

I think there has been talk of making $.Deferred async, but I don't know what the current status is. So, maybe we can make some headway there.

This would be a significant breaking change for jQuery, and there are internal usages of Deferreds that rely on the synchronous behavior (especially in animations), so there has been a lot of pushback on moving to asynchronous resolution.

I think this is something that can still happen though.

@domenic
Copy link
Member Author

domenic commented Oct 29, 2012

Where do we all stand on this?

I'm +1, but it is not crucial to me.

@briancavalier
Copy link
Member

@domenic I'm feeling kind of unsure on this one right now. I did notice, though, that a promises-a branch has appeared in the jQuery repo, and code appears to be trying to comply with Promises/A ... no sign of async yet, tho (as @wycats indicated, that'd be a much more invasive change).

@briancavalier
Copy link
Member

Is this something we can move forward without? I'm thinking if we get #6 worked out, we can announce this thing publicly. Thoughts?

@lsmith
Copy link
Contributor

lsmith commented Oct 31, 2012

It seems reasonable to me to continue without it, but I'd rather see the spec defined without then.aplus, so it's easy for me to say.

@domenic
Copy link
Member Author

domenic commented Oct 31, 2012

It sounds like there's not near a majority wanting this, so yeah, we can move on without it. I'll continue manually blacklisting $.Deferred.

I don't think we should publicly announce until we have tests, but it shouldn't take me long to port those.

@briancavalier
Copy link
Member

Ok, tests repo created

@domenic domenic closed this as completed Oct 31, 2012
@wycats
Copy link

wycats commented Oct 31, 2012

Sad. 😦

@domenic
Copy link
Member Author

domenic commented Mar 7, 2013

Discussion has been somewhat reopened in slightlyoff/Promises#40. Implementer feedback appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants