Version 2 Review

[kelunik]

@trowski @bwoebi @rdlowrey I reviewed Amp v2.0 and here are my comments.

Internal\WhenQueue

How important is the layer of indirection? If we're worried about the call stack, should we instead resolve promises asynchronously?

It could also be renamed to a more generic name like CallbackQueue or CallQueue.

Internal\Placeholder

Annotation for $onResolved is callable|\Amp\Internal\WhenQueue|null, should be ?WhenQueue or ?WhenQueue|null instead.

Coroutine

Why does it not accept a callable as constructor parameter in addition of generators?

Emitter

if ($this->resolved) {
    return;
}

^ Does that ignore double resolution silently?

Pause

Do we really need $value there? What about keep_alive = false?

Internal\LazyPromise

Why is this one internal? Why is Amp\lazy suggested to be used only? Why do we even have Amp\lazy? Instead of using arguments to Amp\lazy, those parameters can just be use'd in a closure by the user instead of providing them as arguments.

Amp\wrap vs. Amp\coroutine

It is weird to have both and usage will be confusion for users.

Amp\timeout

I think Amp\timeout shouldn't keep the loop running. The watchers should probably be unreferenced.

Amp\lift

Do we need lift? I always ask that when reading the docs regarding it.

Amp\filter

Amp\filter filters observables now instead of promises.

[bwoebi]

Regarding Emitter, I do not see any case where that branch ever shall be hit … looks like dead code to me which we can remove.

Agree on timeout() and Pause regarding keep-alive.

Why shall Coroutines accept callables? It is only appending () on caller side. Being explicit here is very cheap and thus I do not see any issue with accepting only Generators.

The difference between wrap() and coroutine() is that the one calls rethrow() and the other returns the promise. I'm not sure here. Having wrap reduces possibility that it's forgotten to manually rethrow on the caller side, but at the same time it's more confusing. I wish to hear others opinion here.

/ For the other functions, better ask @trowski

[kelunik]

@bwoebi Just the same reason why we accept callable for Amp\resolve, too. A simpler API if you pass a anon function. /cc @DaveRandom

[bwoebi]

I do not think that was a good decision back then and the status quo [in v2] presents a better solution.

[kelunik]

If you argue that way, we should probably also remove all wrappers like Amp\execute and Amp\onReadable, as they're just one wrapping away.

[trowski]

Internal\WhenQueue is used only if there are multiple callbacks registered on a promise. It's not really about indirection as reducing the amount of logic in promises, only creating an array of callables if needed. I'm indifferent on naming, though __invoke()'s arguments are specific to when(), so…

The annotation in Internal\Placeholder for $onResolved is correct. If anything, it could just be ?callable.

I agree with @bwoebi, the Coroutine constructor should just accept generators.

The path in Amp\Emitter can be reached if the coroutine tries to emit a promise that then fails. See https://github.com/amphp/amp/blob/master/lib/Internal/Producer.php#L60-L64. The observable will then be resolved before the when callback on the coroutine is invoked. It's not ignoring double resolution, rather preventing double resolution of the observable.

Amp\lazy() is something I copied from JS implementations, but have never actually used. It is a nice way to create a set of dependencies even if you are not aware of what will be used - the typical example is module loading - but as you suggest there are other ways. We should make it a non-internal class like Pause if we want to keep it, or just drop the concept entirely.

I agree, Amp\timeout() should not keep the loop running.

The value for Amp\Pause is pretty pointless… a keep-alive argument would be much more useful. What should the promise resolve with? Maybe the the time slept?

Amp\lift() is used in Amp\map(), but beyond that I've never used it… I could just roll the code into Amp\map().

Amp\filter() could be renamed and the old function added again (or vice versa). So far the missing function hasn't come up, so I wonder if it was needed.

[kelunik]

@trowski I'm not sure whether observables should completely fail if one emit fails.

Regarding lazy, I have a use case for it. It's useful for a session middleware that assigns the session to a local request variable and loads it only on first need. I just think the additional indirection of a function is not needed here.

Amp\Pause should probably just resolve to void, so null. I'm fine with the time, too, but usually you just do yield new Pause($ms), so you have the time there already.

I have seen the use of Amp\lift, but IMO it's hard to follow what the code actually does.

I don't know whether I used Amp\filter, but I think it could make sense to keep BC here, so it's easier to upgrade for people using version 1 currently.

[joshdifabio]

The API looks really good. Great review too @kelunik.

Regarding the proxy functions - such as execute() and stop() - are they really necessary? They increase the surface area of the public API with all the costs that incurs.

E.g.

execute($fn);
// vs
Loop::execute(wrap($fn));

Calling the Loop methods directly also seems much more explicit to me, whereas execute(), etc., present additional stuff for developers to learn and understand.

[kelunik]

@bwoebi @trowski What do you think about the proxy functions?

[bwoebi]

I disagree that it is additional stuff to learn. Developers don't need to learn the Loop stuff unless they want to do very advanced things. Thus the only thing devs have to learn is the proxy functions which handily auto-wrap for you.

[kelunik]

@bwoebi They have to, assuming Amp isn't the only implementation. It's two ways of doing the exact same thing without providing real benefit. Most onReadable / onWritable handlers etc. aren't coroutines anyway.

[bwoebi]

@kelunik Why? Other implementations will need to use their own proxy/the loop funcs directly, but someone who uses Amp can use libraries from other impls without needing to know about the loop funcs??
Only in case where you would want to use the other library, sure. But why would you?

[kelunik]

can use libraries from other impls without needing to know about the loop funcs

I'd say people should know about Loop, they should use it.

Only in case where you would want to use the other library, sure. But why would you?

Any library you use and might dig into might use another async helper library.

[kelunik]

@bwoebi In general the discussion should be why we should have them instead of why we might not need them. The public API should be as minimal as possible. That's not only less to document, less bugs, it's also less to read and learn when starting.

[bwoebi]

We're talking about the same - We should have it, because it isn't an extra API (i.e. I disagree people should need to know about Loop) and is the most useful API for people.

[kelunik]

@bwoebi We're not talking about the same. I want to remove those functions, you want to keep them.

[joshdifabio]

I disagree people should need to know about Loop

Are you suggesting 1) people shouldn't need to know that the loop is there or 2) that they simply shouldn't need to know the interop loop API?

If 1) then what do we expect people to think stop() means for example? Stop what? The description of the function even says Stop the loop, with a link to the interop docs.

If 2) then what makes these proxy functions superior to the interop Loop methods? Are they simply there to provide insulation or are they there to add coroutine support? As a user I think I'd always code against interop so that I could swap out implementations, but using amp I'd be a bit confused about why I had an additional way of operating on the loop.

[bwoebi]

I mean 2), and it has both benefits, a bit of insulation (e.g. in case the spec ever changes!?) Both.

Also, you anyway won't be able to swap simply. the Promise API. The helper functions. The coroutine class. Too many dependencies.

[kelunik]

a bit of insulation

That gains us nothing. The spec should be pretty stable at this point.

Also, you anyway won't be able to swap simply. the Promise API. The helper functions. The coroutine class. Too many dependencies.

I still think we should separate those into separate packages. The helper functions and coroutine class are useful for any promise implementation.

[bwoebi]

I still think we should separate those into separate packages.

That gains us nothing. People may include amp without using all the available functions and classes.

[kelunik]

It does. We can update observables should a need to standardize them arise without requiring all packages using coroutines to upgrade to a new major version.

[bwoebi]

That's an utopian dream. We could then as well just create a separate repo for every single function, because they might need to be changed somewhen.

Realistically, you end up having observables at least as an indirect dependency if you use anything from amphp.

[joshdifabio]

Realistically, you end up having observables at least as an indirect dependency if you use anything from amphp.

I think observables are a separate problem to the proxy functions because they have a clear reason to exist, but I certainly think they would benefit from being standardised in the same way that Promises have been (i.e. an interface).

[trowski]

I think the proxy functions are unnecessary magic and largely just fluff (as in the case of Amp\stop(), Amp\enable(), and other like functions that are nothing more than wrappers around Loop::*()). Amp\wrap() should be required when a callable should be run as a coroutine. Additionally, Amp\wrap() should again call Amp\rethrow() if the function returns any promise. This way a callback can invoke something async without having to be a coroutine itself.

We currently have a situation where callbacks are sometimes able to be coroutines, but usually they are treated as regular functions. It's difficult to remember under what circumstances a function will be run as a coroutine, so I'd rather remove this magic and require users to be explicit by wrapping callbacks with Amp\wrap() when the function will be a coroutine.

[kelunik]

I think we should separate the stream functions into either their own namespace of give them a prefix. map makes sense for both, streams and promises. Same for filter.

[kelunik]

All important issues have been resolved.