Coder Social home page Coder Social logo

Comments (22)

jdanyow avatar jdanyow commented on August 23, 2024

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".

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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.)

from event-aggregator.

singledigit avatar singledigit commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

@digitzfone I think you meant detached.

from event-aggregator.

singledigit avatar singledigit commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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.

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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

from event-aggregator.

singledigit avatar singledigit commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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.

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

I am absolutely fine with that :)

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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

from event-aggregator.

jdanyow avatar jdanyow commented on August 23, 2024

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?

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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.

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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();

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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.

from event-aggregator.

EisenbergEffect avatar EisenbergEffect commented on August 23, 2024

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.

from event-aggregator.

tlvenn avatar tlvenn commented on August 23, 2024

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.

from event-aggregator.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo 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.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.