Comments (25)
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
from amp.
@bwoebi Just the same reason why we accept callable
for Amp\resolve
, too. A simpler API if you pass a anon function. /cc @DaveRandom
from amp.
I do not think that was a good decision back then and the status quo [in v2] presents a better solution.
from amp.
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.
from amp.
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.
from amp.
@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.
from amp.
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.
from amp.
@bwoebi @trowski What do you think about the proxy functions?
from amp.
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.
from amp.
@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.
from amp.
@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?
from amp.
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.
from amp.
@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.
from amp.
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.
from amp.
@bwoebi We're not talking about the same. I want to remove those functions, you want to keep them.
from amp.
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.
from amp.
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.
from amp.
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.
from amp.
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.
from amp.
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.
from amp.
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.
from amp.
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).
from amp.
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.
from amp.
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
.
from amp.
All important issues have been resolved.
from amp.
Related Issues (20)
- Confusing diagram in README HOT 6
- Return self in Future::ignore HOT 1
- UnhandledFutureError is hard to debug HOT 2
- Migration guide for combinator functions HOT 5
- FiberError: Cannot switch fibers in current execution context on Laravel HOT 4
- 32 bit PHP: Return value of Amp\Loop\NativeDriver::now() must be of the type int, float returned HOT 5
- Functions should require callable instead of closures HOT 10
- Any way to create or get a context for coroutine. HOT 5
- Version confusion HOT 8
- Shouldn't Future::finally() pass the future data to the provided callback? HOT 7
- Event loop terminated without resuming the current suspension (the cause is either a fiber deadlock, or an incorrectly unreferenced/canceled watcher) HOT 3
- Future combinators HOT 8
- When to not to use `async`?
- Comparison with ReactPHP HOT 8
- The docs not up to date? HOT 3
- Where is this `any()` function?
- Interception of pause / resume future HOT 2
- Add Payload::readEntireBody? HOT 3
- Symfony Messenger Integration HOT 3
- Using AMPHP with Psalm HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from amp.