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

Bug: EventManager Uses Incorrect Compile-Time Generic Type for Event Dispatching #22

Open
Matthiee opened this issue Sep 8, 2024 · 1 comment · May be fixed by #25
Open

Bug: EventManager Uses Incorrect Compile-Time Generic Type for Event Dispatching #22

Matthiee opened this issue Sep 8, 2024 · 1 comment · May be fixed by #25
Assignees
Labels
Breaking Breaking change in the API bug Something isn't working

Comments

@Matthiee
Copy link
Member

Matthiee commented Sep 8, 2024

The following method uses the incorrect compile-time generic type.

When a redirect factory constructor is used, the generic type will be the base class, instead of the instance type.

This causes the wrong event handler to be used.

/// Dispatches the given [event] to the registered [EventHandler]'s.
Future<void> dispatch<TEvent extends DomainEvent>(
TEvent event, [
DispatchStrategy? dispatchStrategy,
]) async {

Sample:

sealed class BaseEvent implements DomainEvent {
  factory BaseEvent.added() = Added;

  factory BaseEvent.removed() = Removed;
}

class Added implements BaseEvent {}

class Removed implements BaseEvent {}

void checkEvent<TEvent extends BaseEvent>(String name, BaseEvent event) {
  print('$name handler');
  print('TEvent: $TEvent');
  print('Instance: ${event.runtimeType}');
}

extension on EventManager {
  Future<void> dispatchBaseEvent(BaseEvent event) => dispatch(event);
}

Future<void> main() async {
  final mediator = Mediator.create();

  mediator.events.on<Added>().subscribeFunction((e) => checkEvent('added', e));
  mediator.events
      .on<Removed>()
      .subscribeFunction((e) => checkEvent('removed', e));
  mediator.events
      .on<BaseEvent>()
      .subscribeFunction((e) => checkEvent('base', e));

  await mediator.events.dispatchBaseEvent(BaseEvent.added());
  await mediator.events.dispatchBaseEvent(BaseEvent.removed());
}

Output:

base handler
TEvent: BaseEvent
Instance: Added

base handler
TEvent: BaseEvent
Instance: Removed

Expected:
The events should be handled by the Added, Removed event handlers instead of the BaseEvent handler.

@Matthiee Matthiee added bug Something isn't working Breaking Breaking change in the API labels Sep 8, 2024
@Matthiee Matthiee self-assigned this Sep 8, 2024
@Matthiee
Copy link
Member Author

After some consideration, this is working as intended. Changing this would enable a specific use case while breaking another more valid one.

Workaround

Make the following changes in the code sample above.

Update the events to not implement DomainEvent on the lowest type. Introduce a base class in between.

sealed class SomeEvent {
  factory SomeEvent.added() = AddedEvent;

  factory SomeEvent.removed() = RemovedEvent;
}

abstract base class SomeEventBase implements DomainEvent {}

final class AddedEvent extends SomeEventBase implements SomeEvent {}

final class RemovedEvent extends SomeEventBase implements SomeEvent {}

Update the extension method

extension on EventManager {
  Future<void> dispatchSomeEvent(SomeEvent event) {
    // Switching on the instance type will cause the dispatch<Event>
    // to use the correct generic type.
    // Analyzer will complain if you don't do this or forget a case
    switch (event) {
      case AddedEvent():
        return dispatch(event);

      case RemovedEvent():
        return dispatch(event);
    }
  }
}

Now the output for

  await mediator.events.dispatchSomeEvent(SomeEvent.added());
  await mediator.events.dispatchSomeEvent(SomeEvent.removed());

Is the following:

added handler
TEvent: SomeEvent
Instance: AddedEvent

removed handler
TEvent: SomeEvent
Instance: RemovedEvent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Breaking change in the API bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant