felixfbecker / php-language-server

PHP Implementation of the VS Code Language Server Protocol 🆚↔🖥
ISC License
1.14k stars 185 forks source link

Switch from Sabre\Event to ReactPHP #359

Open felixfbecker opened 7 years ago

felixfbecker commented 7 years ago

ReactPHP can use OS level event loops and has more interoperability with libraries like RxPHP. The event loop is not global like Sabre's.

staabm commented 7 years ago

When checking React you should also check amphp which is developed more active and by more people. It also support libuv, libev etc

felixfbecker commented 7 years ago

What makes you think that?

image

image

https://packagist.org/packages/amphp/amp/stats https://packagist.org/packages/react/event-loop/stats

felixfbecker commented 7 years ago

I see you are a contributor to amphp, if you see any advantages amphp has, I would be curious to hear

staabm commented 7 years ago

Under the amphp umbrella the guy behind icicle.io (another async php framework) and a few php core contributors joined forces to create a react php successor mainly with performance in mind as react php was too slow for them.

They created a great php based webserver https://github.com/amphp/aerys which proves the high performance usecases.

These guys are also the ones behind https://github.com/async-interop which aims to provide common abstractions so you can mix components of the different frameworks.

Regarding react: last time I looked up the github Organisation it was rather inactive. Today I can see that they are working again on the libs, so this argument seems no longer be accurate.

The number of downloads on packagist show more that the components are used instead of beeing actively developed. So take those with a grain of salt.

Reactphp code most of the time is promise and callback based, which leads to callback hell style unreadable code. In amphp you can do stuff like yield promises which helps a lot in maintaining your code and reason about it.

felixfbecker commented 7 years ago

Thanks for the info. async-interop seems to be dead though. How do the two compare feature-wise?

felixfbecker commented 7 years ago

No support by RxPHP would be a deal breaker for me: https://github.com/ReactiveX/RxPHP/blob/master/src/Scheduler/EventLoopScheduler.php

bwoebi commented 7 years ago

With amp v2 (soon to be released; see the master branch), we're natively supporting React Promises in our coroutines and combinators, as well as also providing a proper React adapter (https://github.com/amphp/react-adapter) which will enable you to also use RxPHP too.

Also, amp provides a global event loop, which is superior here (no need to DI it everywhere; there anyway typically only ought to be a single event loop).

Yeah, async-interop is dead, amp v2 is going to be based on top of it.

felixfbecker commented 7 years ago

I don't 100% see how support for ReactPHP Promises helps with supporting RxPHP Schedulers? I wouldn't see global singleton event loop as that much of a benefit, I generally disliked that part of Sabre's API. I don't have a problem with passing explicit dependencies. You can make the argument of "typically only a single x" for a lot of things, Tracers, DB connections, ...

bwoebi commented 7 years ago

Right, but while you can execute multiple queries on multiple connections simultaneously (non-blocking), you can have only one event loop running at the same time. [Otherwise it blocks other loops.] That's why we went with that design choice.

100% support for ReactPHP Promises and the React Loop, will enable you to use RxPHP fully.

felixfbecker commented 7 years ago

That's true, but there could be cases where you want to mock it in tests per-test.

Did you look at the scheduler class I linked? It's not related to promises.

Are there any other feature differences? Better perf is good of course

bwoebi commented 7 years ago

That's why we have https://github.com/amphp/phpunit-util You require that repo and add the listener into the phpunit file (see the readme).

The scheduler is using the react loop though, which we have an adapter for (as linked above).

Better performance is definitely a goal of Amp (and perf is already very good - better than React). Apart from that, I consider our libraries a bit more mature and feature complete than Reacts. Still - if you go with Amp, you still can use all the react libraries you wish to via the adapter.

felixfbecker commented 7 years ago

Looking at the docs more (at least the available) I gotta say as a JS dev, I don't like the promise implementation very much. It is basically repeating the same mistakes and iterations JS went through until the community settled on Promises/A+. Error-first callbacks were exactly what we wanted to move away from, you want one catch at the end of your chain. Deferred objects were replaced by the revealing constructor pattern. The argument that the JS pattern doesn't make sense in a language with generators is weird given that JS has generators and coroutines too, and promises are what powers them. ReactPHP stays close to Promises/A.

The streams + loop API with the "cancel ID" looks a bit weird too.

Could you elaborate a bit why you thing amp is more mature / feature complete?

trowski commented 7 years ago

JS didn't have generators and coroutines when Promises/A+ was developed, and now they are stuck with them. There's no reason to chain callbacks on promises in a world with coroutines. Amp's promises reflect this: promises should be yielded in coroutines or passed to combinator functions (such as Amp\Promise\all()). Promise::onResolve() should almost be considered internal – only to be used by library code.

Instead of writing callbacks, code is written like synchronous code, only interjecting a yield where a function returns a promise. Exceptions are caught using a try/catch block instead of defining a callback in catch at the end of a chain.

If you're really insistent on then-chaining, I plan on releasing a library implementing both React's and Amp's promises in a single object.

The loop API is more like that of C event-loop libs and supports features that React's does not, such as multiple watchers on the same stream and signal handling.

As @bwoebi pointed out, an advantage to using Amp is that you have access to any lib written for React as well as any written for Amp.

Amp's libraries are generally more fully-featured than React's. The DNS library automatically reads your local resolver config instead of relying on Google's resolver by default. Aerys is a production-ready HTTP/2 & HTTP/1.1 + websocket server including middlewares, routing, and performance and security features. Amp has an async MySQL, Postgres, Redis, Stomp, and Beanstalk libs. amphp/parallel creates workers using other processes or threads that allows slow or blocking work to be done in a way that does not block the main process.

RxPHP does not require React's event loop for scheduling. Any function can be passed for creating timers, using the React event-loop is only a convenience feature. If using Amp, an instance of ReactAdapter can be given or a function calling Loop::delay() can be defined.