Initial release #2

maxime-rainville · 2024-11-10T21:46:29Z

This Pull request provides the initial implementation for the module.

Parent issue

marwan38

Generally looks good, though a few things I'd personally resolve.

An important bit I want to discuss is:

Dealing with futures explicitly, as in, returning them where relevant in case we ever want to do something with them.
Clearly distinguishing between amphp and revolt. We're mixing APIs - we should be intentional with what's used and where. At the moment, you don't need amphp at all.. you can drop it completely. If we keep it (and tie ourselves into it) then we should utilise their APIs. General feeling of this repo is that it doesn't need amphp. EDIT: If we want Futures to be our API then yes, we should stick completely to the amphp apis
We need to consider signals and event cancellation and referencing. All EventLoop::* apis return a callbackId that you can use to do many things, a few of them: unreference the callback, cancel the callback, etc.. These should probably be part of the higher API of this library.
The API should expose an API for cancellations for library consumers: https://amphp.org/amp#cancellation

marwan38 · 2024-11-12T05:05:59Z

.github/workflows/ci.yml

marwan38 · 2024-11-12T05:07:46Z

examples/basic-usage.php

+$listenerProvider->addListener(UserCreatedEvent::class, function (UserCreatedEvent $event) {
+    // Simulate async operation
+    EventLoop::delay(1, fn () => null);
+    echo "Sending welcome email to {$event->email}\n";


This echo is meant to be within the delay callback I believe. or do you mean to use the delay() function to [async] block?

marwan38 · 2024-11-12T05:10:03Z

src/AsyncEventDispatcher.php

+    public function __construct(
+        ListenerProviderInterface $listenerProvider
+    ) {
+        $this->listenerProvider = $listenerProvider;


Nitpick: construction promotion or whatever it's called.

public function __construct(private Type $var){}

src/AsyncEventDispatcher.php

marwan38 · 2024-11-12T05:19:16Z

src/AsyncEventDispatcher.php

+            EventLoop::queue(function () use ($event, $listener) {
+                $listener($event);
+            });
+        }


I'm still not clear on when to use amp and when to use the lower level api of Revolt.

awaitAll( array_map( fn ($listener) => async(fn() => $listener($event)), $listeners ) )

for example would be the amphp api. We're also using the 2 APIs without clarification (compare this to dispatchStoppableEvent for example.

marwan38 · 2024-11-12T05:21:33Z

src/AsyncEventDispatcher.php

+            }
+        });
+
+        return $event;


I wonder if we should be returning the futures?
// @return Future<StoppableEventInterface>[] for example.

That's a good idea.

For context, the stated return type for the dispatch method according to PSR-14 is @return object The Event that was passed, now modified by listeners.

So strictly speaking we should be returning the same event we got as an input. In practice, this will be useless because our listeners will be executed later, so they will not have had a chance to do anything to our event yet. I think returning a Future make sense here.

We can stick to the standard and expose a getFuture() method on the events. You did say that an Event could be anything under this standard, so it makes perfect sense to add this to the event itself.

src/AsyncEventDispatcher.php

src/ListenerProvider.php

marwan38 · 2024-11-12T05:25:29Z

tests/AsyncEventDispatcherTest.php

+        $completed = false;
+
+        $this->listenerProvider->addListener(TestEvent::class, function (TestEvent $event) use (&$results) {
+            EventLoop::delay(0.1, fn () => null);


delay(0.1); echo 'after waiting 0.1 seconds';

I don't really want my test to be echoing things to the console though.

It was just pseudocode, perhaps I misunderstood your test. I was just pointing out that the EventLoop::delay() here does almost nothing.. I thought you were specifically trying to block execution with a sleep (delay in the context of amphp).

maxime-rainville · 2024-11-12T20:32:39Z

composer.json

+            "@test"
+        ]
+    },
+    "license": "MIT",


I used BSD-3 for my other project. MIT is essentially the same thing.

Claude decided to use MIT. Probably should add a LICENSE.MD file.

src/AsyncEventDispatcher.php

maxime-rainville · 2024-11-12T20:55:59Z

src/AsyncEventDispatcher.php

+            }
+        });
+
+        return $event;


That's a good idea.

For context, the stated return type for the dispatch method according to PSR-14 is @return object The Event that was passed, now modified by listeners.

So strictly speaking we should be returning the same event we got as an input. In practice, this will be useless because our listeners will be executed later, so they will not have had a chance to do anything to our event yet. I think returning a Future make sense here.

src/ListenerProvider.php

maxime-rainville · 2024-11-12T21:00:17Z

tests/AsyncEventDispatcherTest.php

+        $completed = false;
+
+        $this->listenerProvider->addListener(TestEvent::class, function (TestEvent $event) use (&$results) {
+            EventLoop::delay(0.1, fn () => null);


I don't really want my test to be echoing things to the console though.

…ke disptach cancellable

maxime-rainville · 2024-11-13T03:05:34Z

@marwan38
I think I answered most of you feedback.

I made the dispatch method return a Future. I also added the ability to send a cancellation to the dispatch.

marwan38 · 2024-11-13T12:14:39Z

@marwan38 I think I answered most of you feedback.

I made the dispatch method return a Future. I also added the ability to send a cancellation to the dispatch.

Nice, I did leave an updated comment on that regarding futures. I'll touch on it again to emphasize: We can expose a getFuture() on the events to get the futures and sticking to the PSR14 spec. It might make for a better API too.

LGTM and ready to approve, but, you need to consider ☝️ first.

maxime-rainville · 2024-11-13T20:11:00Z

We can stick to the standard and expose a getFuture() method on the events. You did say that an Event could be anything under this standard, so it makes perfect sense to add this to the event itself.

We could create our own AsyncAwareEvent class/interface and get the dispatcher to handle that one differently ... the same way that it's handling StoppableEvents differently.

I did update the dispatch method to have the dispatch(T event): Future<T> signature. I think that effectively accomplishes the same thing as having an specialized Event class with a getFuture() method.

Maxime Rainville and others added 11 commits November 11, 2024 10:43

Initial commit

53948d3

Allow workflow to be run manually

5237f52

Fix build issue

5465fa7

Target master instead of main

3d23fa8

Fix linting warning

a9d1322

Add CI badge

9d96651

Fix the CI badge

9b49c9c

Make explicit that we provide a psr/event-dispatcher-implementation

3827298

Remove unused import

375101d

Remove .gitkeep file

b454b26

Run builds on everything

65e0f25

maxime-rainville mentioned this pull request Nov 10, 2024

Implement an Event mangement system for Silverstripe CMS archiprocode/silverstripe-revolt-event-dispatcher#1

Merged

marwan38 reviewed Nov 12, 2024

View reviewed changes

maxime-rainville commented Nov 12, 2024

View reviewed changes

Maxime Rainville added 4 commits November 13, 2024 12:54

Apply peer review feedback + update dispatcher to return Futures + ma…

cc84b4e

…ke disptach cancellable

fix linting issue

6dd1ee7

set license to BSD-3

b1b7185

Update example

a191094

marwan38 approved these changes Nov 15, 2024

View reviewed changes

maxime-rainville merged commit e33f638 into 0 Nov 17, 2024
8 checks passed

maxime-rainville deleted the master branch November 17, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial release #2

Initial release #2

maxime-rainville commented Nov 10, 2024 •

edited

Loading

marwan38 left a comment •

edited

Loading

marwan38 Nov 12, 2024

marwan38 Nov 12, 2024

marwan38 Nov 12, 2024

marwan38 Nov 12, 2024

marwan38 Nov 12, 2024

maxime-rainville Nov 12, 2024

marwan38 Nov 13, 2024

marwan38 Nov 12, 2024

maxime-rainville Nov 12, 2024

marwan38 Nov 13, 2024

maxime-rainville Nov 12, 2024

maxime-rainville Nov 12, 2024

maxime-rainville Nov 12, 2024

maxime-rainville commented Nov 13, 2024 •

edited

Loading

marwan38 commented Nov 13, 2024 •

edited

Loading

maxime-rainville commented Nov 13, 2024

Initial release #2

Initial release #2

Conversation

maxime-rainville commented Nov 10, 2024 • edited Loading

Parent issue

marwan38 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville commented Nov 13, 2024 • edited Loading

marwan38 commented Nov 13, 2024 • edited Loading

maxime-rainville commented Nov 13, 2024

maxime-rainville commented Nov 10, 2024 •

edited

Loading

marwan38 left a comment •

edited

Loading

maxime-rainville commented Nov 13, 2024 •

edited

Loading

marwan38 commented Nov 13, 2024 •

edited

Loading