subscribe should return subscriptionInfo object with an unsubscribe/cancel function

[tlvenn]

The api right now around how to un subscribe is not really obvious and I see many people in Gitter asking how to do it which lead to something like this:

var subscribe = ea.subscribe(''test"); // Subscribe
subscribe(); // Unsubscribe

Which is really weird when you look back at the code and does not reflect at all what is really going on.

Ideally, the Event Aggregator should return a subscription object such as:

{
  channel: 'test',
  subscribedAt: dateProperty,
  unsubscribe: function() {...}
}

So people can do:

var sub = ea.subscribe(''test"); // Subscribe
sub.unsubscribe(); // Unsubscribe

What do you think ?

[jdanyow]

Another thing that might be worth considering is returning a "subscription/cancellation token" (number) rather than a function. Breeze uses this pattern as well as setTimeout and setInterval. Wondering if it's more "light-weight".

[EisenbergEffect]

It would be if it avoided creating the closure as part of the dispose code. Currently our EA creates a closure and so would the above mechanism, most likely. However, I'm not too concerned about it in this case because you would have to have a ton of messages for it to matter. (That's one reason we need to change the binding system. It creates these closures in the same way...and that's a case where it really does matter.)

[singledigit]

In following the pattern below

var subscribe = ea.subscribe(''test"); // Subscribe
subscribe(); // Unsubscribe

I have setup an EA to subscribe and then call the unsubscribe in the detach function of my view-model. However this doesn't seem to work at all.

Users.js

import {inject} from 'aurelia-framework';
import {HttpClient} from 'aurelia-fetch-client';
import {EventAggregator} from 'aurelia-event-aggregator';
import 'fetch';

@inject(HttpClient, EventAggregator)
export class Users{
  heading = 'Github Users';
  users = [];

  constructor(http, eventAggregator){
    http.configure(config => {
      config
        .useStandardConfiguration()
        .withBaseUrl('https://api.github.com/');
    });

    this.http = http;
    this.po = eventAggregator;

    this.disposeSubscription = this.po.subscribe('test-message', payload => {
      console.log(`Message from subscription on user is "${payload}"`);
      });
  }

  activate(){
    return this.http.fetch('users')
      .then(response => response.json())
      .then(users => this.users = users);
  }

  detach(){
    this.disposeSubscription();
  }
}

Welcome.js

import {inject, computedFrom} from 'aurelia-framework';
import {EventAggregator} from 'aurelia-event-aggregator';

@inject(EventAggregator)
export class Welcome{
  heading = 'Welcome to the Aurelia Navigation App!';
  firstName = 'John';
  lastName = 'Doe';
  previousValue = this.fullName;

  constructor(eventAggregator){
    this.po = eventAggregator;

    this.po.publish('test-message', 'sent from the welcome page');
  }

  //Getters can't be observed with Object.observe, so they must be dirty checked.
  //However, if you tell Aurelia the dependencies, it no longer needs to dirty check the property.
  //To optimize by declaring the properties that this getter is computed from, uncomment the line below.
  //@computedFrom('firstName', 'lastName')
  get fullName(){
    return `${this.firstName} ${this.lastName}`;
  }

  submit(){
    this.previousValue = this.fullName;
    alert(`Welcome, ${this.fullName}!`);
  }

  canDeactivate() {
    if (this.fullName !== this.previousValue) {
      return confirm('Are you sure you want to leave?');
    }
  }
}

export class UpperValueConverter {
  toView(value){
    return value && value.toUpperCase();
  }
}

If I go to users, then go to welcome. the console prints: Message from subscription on user is "sent from the welcome page"

It is also the same if I use deactivate() in the skeleton app.

[EisenbergEffect]

@digitzfone I think you meant detached.

[singledigit]

Yes, your right I do mean detached. However, it still seems to be an issue. In my app I tried detached and deactivate and neither seem to work.

[EisenbergEffect]

Right, the reason is because the navigation lifecycle is going to run like this:

• old screen => canDeactivate
• new screen => constructor
• new screen => canActivate
• old screen => deactivate
• new screen => activate
• old screen => detached
• new screen => attached

So, because you are publishing the message in the constructor, it will be received by the old screen because the new screen's constructor is executed before the old screen is deactivated. The reason for this is that if the new screen returns false for a potential canActivate call, then we're in a bind if we've already deactivated the old screen, which should not have been deactivated.

[tlvenn]

So the issue has been closed and the API remains the same ? Not sure I understand what happened here.

[singledigit]

I follow, It isn't an issue. It was an event order problem. My new constructor was firing a publish message before the old detached was able to dispose of the subscription. I need to reverse the order. So I will move my dispose to the old deactivate and move my publish to the new activate.

[EisenbergEffect]

@tlvenn Sorry for the confusion. We aren't planning to change the api at this time.

[tlvenn]

And what is the reasoning behind not trying to make the API clearer and more readable ? Pretty much all pubsub system and even Observable follow the same pattern of returning a subscription you can call dispose() or unsubscribe() on.

As i outlined above, beside being a pretty bad design choice imho, it's confusing as hell for any people new to Aurelia as it is absolutely counter intuitive and against everything they are used to.

[EisenbergEffect]

I'll tell you what...if you sign our cla and submit a PR with unit tests, I will accept the design change. It needs to happen within the next 10 days or so.

[tlvenn]

I am absolutely fine with that :)

[tlvenn]

Actually I believe I have signed the CLA a long time ago..

[tlvenn]

btw @digitzfone please create your own issue next time ;)

[jdanyow]

Is the API changing to return this?

{
  channel: 'test',
  subscribedAt: dateProperty,
  unsubscribe: function() {...}
}

I don't find any of the above^^^ to be useful. The existing EventAggregator can easily be wrapped to provide this API.

If we are going to make a change I propose we match the setTimeout, setInterval, setImmediate, requestAnimationFrame APIs. They all work the same way, there's a method for "setting" and a method for "clearing". Calling the "set" method returns an ID (a number) which can be passed to the "clear" method.

This pattern applied to the EventAggregator would look like this:

// subscribe:
let subscription = ea.subscribe(LocaleChangedEvent, callback);

// unsubscribe:
ea.unsubscribe(subscription);

Implementation detail: this design would remove the closure involved with creating the "dispose" function by returning the index into the callbacks array as the "subscription id"

That said, I find the current API to be the simplest of all the proposed alternatives.

Thoughts?

[tlvenn]

Hi Jeremy,

I dont think it's really fair or appropriate to try to compare those rather simple JS functions that are completely autonomous with a pub sub system where you create channels, consumers and producers which use those channels to communicate.

And as far as I know, most are designed the way I describe more or less.

Furthermore I dont think it's desirable that the subscription cannot by itself unsubscribe because it would lead to injecting the ea pretty much everywhere where you could potentially need to do it.

[tlvenn]

The existing EventAggregator can easily be wrapped to provide this API.

Ya I agree but the issue is that out of the box it does not and everyone who tries to leverage the EA will ponder how to unsubscribe.

And really, am i the only one thinking that having this code:

sub();

Is completely unreadable ?? As far as i know, I am calling a constructor while actually it's kinda a destructor so it completely hide the purpose in a very dangerous way.

[EisenbergEffect]

Before we commit to a final decision, let's consider:

• One thing I also wanted to do was create a service for working with DOM events, so that it was easier to subscribe/unsubscribe from those. This isn't necessary though, just an idea.
• We want the system to be as efficient as possible. So, no unnecessary creation of objects/arrays/closures.
• If it's possible to make it consistent with any existing apis, either web standards or our internal apis, that would be great.
• It should be simple to use.

@tlvenn BTW The sample code you are showing is much more readable when you write it the way I do when I use it:

let unsubscribe = ea.subscribe(SomeEvent, callback);
unsubscribe();

[tlvenn]

When you know what to expect because you know the API, yes it would be clearer but still having unsubscribe assigned to a call to the subscribe method is little bit weird.

And unfortunately you cant really control how people gonna name their variables but you do control the naming of the API methods and so you should try to help them as much as possible to write readable clean code.

[EisenbergEffect]

It is a matter of opinion. Obviously I don't find it strange at all ;) It also seems more light-weight than your proposal, though not as light-weight as @jdanyow 's. I'm inclined to think that in all three approaches, the developer would have to read the documentation to understand the pattern anyway. The problem today is that there isn't any real example of disposing of a subscription at all in the docs, which makes it harder to learn in general.

I do like the idea of being able to pass around the subscription/disposal object without needing the ea for unsubscribe. So, I think that's a point against @jdanyow 's idea.

So, there are trade offs.

@tlvenn What could push me over the edge to your approach would be if you could show that it performed better than just returning a function. Or that the memory pressure was significantly reduced. To do that you would need to write some benchmarks. You can see our benchmark app repo for that. Start by submitting some PRs to the benchmark repo to add new benchmarks for the existing event aggregator. After that, create a fork of the current EA and make your changes (in this case along with benchmark forks) and then compare the performance and memory of both versions. If you can show that returning an object is more efficient, then that's a good argument. Otherwise, I'm not sure the advantages are great enough to make a breaking change to the API, particularly one that we've heard no negative feedback on aside from this issue. Also, we are on a tight timeline now as we approach beta, so it would need to be wrapped up in about two weeks.

[EisenbergEffect]

FYI, you probably don't want to return an ad hoc object. You probably want to create a class to describe the subscription for perf reasons.

[tlvenn]

Hi Rob, I respect your opinion and we could argue that at the end of the day, it's a matter of coding style and to each their own.

But and it's a big one, you are writing public APIs and a framework for other people to consume not for yourself. So it needs to gather traction and to do that, one of the appeal is how easy and clean the code would be and how intuitive the API is.

And that is one thing you sell well when you compare it to NG2 for example.

Overall Aurelia is a pleasure to work with and pretty intuitive. The EA is not, plain and simple.

As for your benchmark question, I would first ask what kind of benchmark do you already have that would prove that the impact of one more object times the live subscriptions a given system on average would have, could significantly impact the memory footprint of the EA that you are willing to tradeoff good naming convention over performance.

If the EA is such a performance bottleneck, you should already have some benchmark in place to measure any improvement you think you are making. Then it should be trivial to branch it and test a new approach.

Unless proven otherwise, I would favour good naming convention that improve code readability.