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

Changing "when" to "onResolve" or "onComplete" #77

Closed
kelunik opened this issue Mar 13, 2017 · 20 comments
Closed

Changing "when" to "onResolve" or "onComplete" #77

kelunik opened this issue Mar 13, 2017 · 20 comments

Comments

@kelunik
Copy link
Member

kelunik commented Mar 13, 2017

It doesn't matter for normal promises, but I think it matters for more advanced use cases where the promise API is just one way of consumption. Looking at a list of functions, onResolve / onComplete are way more expressive than when.

@kelunik
Copy link
Member Author

kelunik commented Mar 15, 2017

Specifically when just tells that it's something event based, but not on which event the callback is fired. Could be when new data is available on a stream or when there's an error or when …. onResolve clearly indicates which event happens.

@J7mbo
Copy link

J7mbo commented Mar 15, 2017

onComplete definitively means finished. Other Apis that make requests typically use onComplete. onResolve is using yet more terminology that makes entry into things like amp even more prohibitive. Complete > Resolve.

@kelunik
Copy link
Member Author

kelunik commented Mar 15, 2017

@J7mbo Somebody mentioned complete would indicate success. I don't necessarily think so, just mentioning in case others feel that way.

@PeeHaa
Copy link
Contributor

PeeHaa commented Mar 15, 2017

complete sounds like the complete action itself instead of the event being fired.

@J7mbo
Copy link

J7mbo commented Mar 15, 2017

@kelunik Well take a look at jQuery (great example, I know!), I think we had onSuccess, onFail and onComplete, the latter was always executed. Then again that's now .success, .error and .always so not entirely a great comparison. Complete is a positive word so maybe it indicates success. How about onFinish?

@kelunik
Copy link
Member Author

kelunik commented Mar 15, 2017

@PeeHaa A promise is a placeholder for the outcome of an operation. And that operation is complete then. It won't change anymore.

@J7mbo They still have complete. I like onComplete more than onFinish, can't really tell why. Neither does really fit with Promise though. And we already have resolve on the Deferred part.

@bwoebi
Copy link
Member

bwoebi commented Mar 15, 2017

Also ping @rdlowrey

@joshdifabio
Copy link
Contributor

joshdifabio commented Mar 15, 2017

In some contexts onComplete makes more sense and in some contexts onResolve makes more sense, but promises resolve, they don't complete. However, I think part of the issue here is that you are implementing Promise directly in places where you shouldn't. For example, I don't think it makes sense for a stream to implement Promise directly as a stream isn't primarily a placeholder for some single future value.

function ($stream) {
    yield $stream; // this doesn't read well at all. What are we even waiting for here?
}

I would prefer this:

function ($stream) {
    yield $stream->awaitEnd(); // or whenEnded()
}

Regarding the name when(), I'm struggling to think of a context in which it makes sense:

a()->when($b); // in English this reads as "do $a when $b", which is the opposite of what the code does

@kelunik
Copy link
Member Author

kelunik commented Mar 15, 2017

@joshdifabio One such example of (optional) emits as a stream is Artax. The main purpose of request is to return the response, but it emits updates as Artax opens sockets, completes TLS handshakes and follows redirects.

@PeeHaa
Copy link
Contributor

PeeHaa commented Mar 15, 2017

@kelunik

And that operation is complete then.

My point was that naming the method complete tells me it's the action if completing something instead of onComplete telling me it's an event based on the completion.

@kelunik
Copy link
Member Author

kelunik commented Mar 15, 2017

Oh, I didn't mean complete as a method name, but the complete in onComplete.

As we have Deferred::resolve, Promise::onResolve makes sense.

@kelunik
Copy link
Member Author

kelunik commented Mar 18, 2017

@bwoebi You wanted 2-3 days to think about it. How do you feel now?

@bwoebi
Copy link
Member

bwoebi commented Mar 20, 2017

I still really don't like it.
I do not feel like it helps understanding anything.
For random API parts, more expressive names are typically fine.
when() however is quite a unique part of the API. It's not your typical callback, but you need to exactly know what it is. Calling it onResolve just tells you a small part of its function. But it lets you ask another question: "When is a Promise resolved? What will resolve it?" As such, you either way need to have a closer look at the documentation, at which point there's no advantage of calling it onResolve() vs when().
when() is simply just short and concise. Moreover when() sounds a bit like then(), suggesting for people from other Promise APIs that it must be related, making it easier for them.
onResolve() feels like a step backwards for me.

@PeeHaa
Copy link
Contributor

PeeHaa commented Mar 20, 2017

Not saying I disagree with the gist @bwoebi, but saying something sounds like something else to make something more clear is weird imo.

@bwoebi
Copy link
Member

bwoebi commented Mar 20, 2017

It's just a single advantage, not the whole argument ;-)
It's more the type of thing which would make me decide on something if all other advantages and disadvantages are about equivalent.

@kelunik
Copy link
Member Author

kelunik commented Mar 20, 2017

I do not feel like it helps understanding anything.

It makes the API consistent. We have Deferred::resolve, why introduce a new term when instead of using the existing API name?

when() however is quite a unique part of the API. It's not your typical callback, but you need to exactly know what it is.

You can bring that argument for every single name. It's not an argument.

when() is simply just short and concise. Moreover when() sounds a bit like then(), suggesting for people from other Promise APIs that it must be related, making it easier for them.

Indeed, and that's an argument against using when(). Because it's really not then(), but rather similar to done(). It's not chainable, you shouldn't throw in it, and it doesn't make sense to return a value from it.

onResolve() feels like a step backwards for me.

Could you explain why it's backwards?

@bwoebi
Copy link
Member

bwoebi commented Mar 20, 2017

It makes the API consistent. We have Deferred::resolve, why introduce a new term when instead of using the existing API name?

The Deferred is just one implementation, which returns a Promise when some method is called. It's a separate API from Promise. It makes sense to model the naming of the Deferred methods after the method(s) of the Promise, but not vice-versa.

You can bring that argument for every single name. It's not an argument.

Most APIs do not have so much complexity behind. - No.

Indeed, and that's an argument against using when(). Because it's really not then(), but rather similar to done(). It's not chainable, you shouldn't throw in it, and it doesn't make sense to return a value from it.

I just disagree. If it were the same, we'd call it then(). Throw/return/chain are just one thing: it transforms the promise. There's the only difference. We do not return a transformed promise. For everything else, it's quite the same thing.

Could you explain why it's backwards?

Our current naming is totally fine and you're proposing something worse…

@kelunik
Copy link
Member Author

kelunik commented Mar 20, 2017

The Deferred is just one implementation, which returns a Promise when some method is called. It's a separate API from Promise. It makes sense to model the naming of the Deferred methods after the method(s) of the Promise, but not vice-versa.

It's the standard implementation. resolve + onResolve work fine together, it's clear from the name that they're related. Can you propose something for Deferred matching when?

Throw/return/chain are just one thing: it transforms the promise. There's the only difference.

It's not the only difference. The second difference is one callback instead of two. And transforming the promise or not is exactly the difference between then and done.

Most APIs do not have so much complexity behind. - No.

Our promises aren't really complex, they're quite simple, as they don't have any chaining or something like that. They just fire a simple callback (or a set of them) on resolution.

@bwoebi
Copy link
Member

bwoebi commented Mar 20, 2017

Can you propose something for Deferred matching when?

resolve perfectly matches whenwhen callbacks are triggered basically upon the state change of the Promise. That state change is triggered by resolving, so that's totally fine.

It's not the only difference. The second difference is one callback instead of two. And transforming the promise or not is exactly the difference between then and done.

Well, I never liked the name done() … But I'd even prefer done() over onResolve() … it's just too clumsy for me...

Our promises aren't really complex, they're quite simple, as they don't have any chaining or something like that. They just fire a simple callback (or a set of them) on resolution.

Tell that someone who's never interacted with Promises in his life ;-)

@kelunik
Copy link
Member Author

kelunik commented Mar 20, 2017

the state change of the Promise

In that case our streams shouldn't extend Promise.

Well, I never liked the name done() … But I'd even prefer done() over onResolve() … it's just too clumsy for me...

Using done is only possible if we use two callbacks, otherwise it's not compatible with other existing implementations.

Tell that someone who's never interacted with Promises in his life ;-)

Not being able to understand things is mostly not due to promises, but rather due to generators or not understanding the event loop is the scheduler.

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

No branches or pull requests

5 participants