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

Add last-value/immediate option #29

Closed
suterma opened this issue Sep 27, 2024 · 26 comments
Closed

Add last-value/immediate option #29

suterma opened this issue Sep 27, 2024 · 26 comments
Labels
enhancement New feature or request

Comments

@suterma
Copy link

suterma commented Sep 27, 2024

Hi and first of all thank you for this great event handling framework.

One issue I came across, was that it does not support the receipt of the "last emitted value" at subscription time, like e.g. the "immediate" feature VueJs-Watchers do have.

Context: I have a handler for stuff, that emits various events, some of the events regularly, some seldom. New users (clients) of such handlers register to the various events, but do not get the "current" state of the respective property automatically.

So, right after subscription, the clients need to get the current state, for each event, via an explicit call. This adds quite some boilerplate code, eg. below, for each event/property:

onDurationChangedSubscription = handler.onDurationChanged.subscribe(
    (duration) => {
        updateDuration(duration); // update the emitted state
    },
);
updateDuration(handler.duration); // get current state

The preferred solution for this would be an additional option to the subscription that initially calls the "updateDuration" function with the last ever emitted value (making the last line in the above example obsolete).

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

That is effectively the kind of feature available in RXJS, whereas this library is more of a replacement for the standard EventEmitter, which does not support any previous state broadcast.

I do see the value in adding such feature though, so I will consider it.

The preferred solution for this would be an additional option to the subscription that initially calls the "updateDuration" function with the last ever emitted value

In RXJS such a functional distinction is made at construction time, and so I might follow the same pattern here, by extending IEventOptions. Need to think about that. What do you think?

@vitaly-t vitaly-t added the enhancement New feature or request label Sep 29, 2024
@suterma
Copy link
Author

suterma commented Sep 29, 2024

Performance-wise the use of an event option at construction time might be best. Thus, you only need to keep a value around, when it's actually intended.

However a subscriber (I like that term better than Observer, like with RXJS), will never know then, because it will not be able to willfully set or even retrieve, this option for itself, when it actually subscribes. So from a usage standpoint, it will be best, if the subscriber can determine at subscription time. I would not go so far as to actually provide the old value for reference, or even a flag whether this is a "retained" value. This would be unhelpful most of the time I guess.

For me, the first, your, idea would be most straightforward, since I control both the emitter and subscriber, thus will be able to handle the situation optimally for each event type anyway.

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

Ok, so I'm thinking now of adding emitLast: boolean flag to ISubOptions interface.

However, it is not clear then what should we do inside once in this case, which takes the same options. Should we treat the last value as the only value or wait for one after, before auto-unsubscribing? 😸

@suterma Any idea?

UPDATE(s)

The more I think about it, the more I believe that if you pass emitLast: true into once method, it should unsubscribe immediately, and not wait for another event to occur. That would seem consistent, logic-wise.

More problems - method toPromise would need to be extended, for consistency, to also support emitLast.

Not such a simple feature after all 😄

@suterma
Copy link
Author

suterma commented Sep 29, 2024

Yeah, to keep things simple, one needs to put a lot of thinking into it :-)

  • I am not so sure about the "last" naming. There could also be none. But I fail to find a better name.
  • Besides once, you could also have "next", or "next-only", to distinguish from "once", but I see your point. However, if one deliberately subscribes with "last", then they should be aware of "last" maybe meaning "immediately what is available".
  • toPromise should be easy, it simply should follow what the above does in the promise-style.

Good luck. I have solved my problem at hand, just so you know, for the time being, using my workaround from the initial post. It's doing fine and maybe, just maybe, is the most simple and straightforward way after all. :-)

@vitaly-t
Copy link
Owner

This library hasn't been updated in ages. I will need to update everything before adding any new feature.

Started with this PR.

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

That PR has been merged, a new version 1.9.1 released with tons of updates and changes, except no code changes, hence the minor version increment. This way we now have a good base to start looking at new features ;)

@vitaly-t
Copy link
Owner

I have started adding this feature in this PR.

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

This feature seems to create a lot of logical problems - race conditions between emitting events, subscribing, notifying and providing the start value. I've been trying to figure it all out, and at the moment I do not see how this will work correctly...

I cannot just send the last value when calling subscribe, because, for example inside once we try to immediately unsubscribe when we get the first value, which in this case would be triggered before a subscription is even created, which is a logical contradiction.

I am now thinking of maybe simplifying it, by not having any new option, and instead have SubEvent support lastEvent read-only property to always expose the last emitted event. This way things will get super-simple and reliable, no issues.

UPDATE

After a bit more thinking, the latter is also not without problems. If I set this property after the event has been emitted to all subscribers, then new subscribers may miss it even though the emit for the event has already been called. And if I set it at the start of the emit, then lastEvent may contain event that your subscriber is about to receive, thus causing event duplication.

@suterma
Copy link
Author

suterma commented Sep 29, 2024

About immediate unsubscription: Could you not return an already cancelled subscription in this case? This is how I return promises, when there is nothing actually to wait for. E.g. here in my app: https://github.com/suterma/replayer-pwa/blob/de02fc5878831b28b28936dd9ced25ef6a9eb936/src/code/media/AudioFader.ts#L370C1-L372C17

If you have a "lastEvent" property, this might be fine, but would not solve my problem, I still would need to have the additional line of code. Querying the event, instead of the emitter object, but effectively no savings for me.

In any case, don't feel obliged to implement this for me personally. It's no deal-breaker for me.

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

Could you not return an already cancelled subscription in this case?

It is not that simple, subscriptions have events they provide, like when you unsubscribe, which isn't possible to do before a subscription is even created.

As per the above UPDATE, adding lastEvent is also problematic, as adding it at the start of emission will result in event duplication, while setting it at the end of emission may result in missed events for new subscribers.

In other words, I am stuck with this feature for the moment, it does not fit into the existing concurrency model, it creates logical/concurrency problems.

P.S. Looking on the bright side, at least I have made a major update of the library with the previous PR, for v1.9.1

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

If you have a "lastEvent" property, this might be fine, but would not solve my problem, I still would need to have the additional line of code. Querying the event, instead of the emitter object, but effectively no savings for me.

Kind of, but you would be able to get the last event before creating a new subscription, and not change the code inside the subscription 😉 That way, I think, it is still quite useful, if you think about it, no need re-emitting anything.

@vitaly-t
Copy link
Owner

I have just published 2.0.0-beta.1 beta version, with lastEvent added to SubEvent. You can try and play with, see if it helps you at all 😉

@vitaly-t
Copy link
Owner

vitaly-t commented Sep 29, 2024

Released v1.10.0, with lastEvent added, plus fixed bug #33 in it. This might be it, as far as this feature goes.

@suterma
Copy link
Author

suterma commented Oct 1, 2024

I am currently trying this out. I see however, that some links in the docs are broken, while I was trying to find the documentation of this new feature: e.g. https://vitaly-t.github.io/sub-events/classes/subevent.html#subscribe

@vitaly-t
Copy link
Owner

vitaly-t commented Oct 1, 2024

Yeah, the updated automatic web documentation started generating different url-s. I have updated all the links now ;)

@suterma
Copy link
Author

suterma commented Oct 1, 2024

Found it: https://vitaly-t.github.io/sub-events/classes/SubEvent.html#lastEvent
And the feature works as advertised.

Thanks for your effort, it's much appreciated!

@vitaly-t
Copy link
Owner

vitaly-t commented Oct 1, 2024

Good! I am closing the issue now, because I am not planning any work in this direction presently. The alternative with subscription promised too many concurrency issues, so we end up with just lastEvent instead, which is very straighforward.

@vitaly-t vitaly-t closed this as completed Oct 1, 2024
@suterma
Copy link
Author

suterma commented Oct 1, 2024

I'll write a "subscribe and initialize with last" function for myself, that I will share with you. This way, I hope, I can get rid of my surplus lines. You might want to include that in your "Extras" or mention somewhere in the docs, or none at all.

@suterma
Copy link
Author

suterma commented Oct 1, 2024

Here is my new "subscript and initialize" method:

function subscribeAndInitialize<T = unknown>(
    event: SubEvent<T>,
    update: SubFunction<T>,
): Subscription {
    const subscription = event.subscribe((value) => {
        update(value);
    });

    const last = event.lastEvent;
    if (last) {
        update(last);
    }

    return subscription;
}

It's quite simple and does what I need in one line to call:

    subscribeAndInitialize(handler.onDurationChanged, (duration: number) =>
        updateDuration(duration),
    );

It does not have the options (as I don't need these) and it's not yet a protype-added function, but you get the idea. Feel free to incorporate this as an additional way to subscribe.

@vitaly-t
Copy link
Owner

vitaly-t commented Oct 1, 2024

If you want to control this on the variable level, you can create your own class for that:

import {SubEvent, Subscription, SubFunction, ISubOptions} from 'sub-events';

class SubEventInit<T> extends SubEvent<T> {
    subscribe(cb: SubFunction<T>, options?: ISubOptions): Subscription {
        const sub = super.subscribe(cb, options);
        if (this.lastEvent !== undefined) {
            cb(this.lastEvent);
        }
        return sub;
    }
}

Otherwise, the full syntax for the custom subscribe function would be:

function subscribeInit<T>(e: SubEvent<T>, cb: SubFunction<T>, options?: ISubOptions) {
    const sub = e.subscribe(cb, options);
    if (e.lastEvent !== undefined) {
        cb(e.lastEvent);
    }
    return sub;
}

@suterma

@suterma
Copy link
Author

suterma commented Oct 1, 2024

Thanks @vitaly-t . I am mostly struggling with typings for the prototype addon now. In c# this was simple. Here, I might go with your proposed derived class. Seems to be the next most straightforward way then. And since all my subscribers will be either have initializations or not, it also matches my scenario.

Strange thing is though, that I can define the usage of the new class in my interface type, then initialize the property in my concrete class using the basic SubEvent type. It's difficult....

Thanks for sharing!

@vitaly-t
Copy link
Owner

vitaly-t commented Oct 1, 2024

I wouldn't spend more time on this. If you need advanced event processing, there is also RXJS 😉

@suterma
Copy link
Author

suterma commented Oct 1, 2024

I like doing things right, and I love TypeScript. It's my spare-time project so I am always thankful when I can learn something. That said, after all, I am going with a simple local function, like the one you posted last, as all my handler code is in the same class anyway.

@suterma
Copy link
Author

suterma commented Oct 1, 2024

Oh, I have one problem still found, and sorry to bother you. For events that do not have a value, the already emitted events seem not get retained, and lastEvent is seemingly "undefined" event if a previous event actually has been emitted.

I explicitly check for undefined, like in your last shared code.

Now it can be argued whether events without value should have the above behaviour in the first place, but I have expected to get the callback called. You could, for example, return null (as opposed to undefined) as the last value in these cases.

If you decide to leave it as is, I suggest to document this "exceptional" behaviour.

@vitaly-t
Copy link
Owner

vitaly-t commented Oct 1, 2024

Technically, it is documented, within lastEvent, that it would be undefined if not emitted yet. So, it just happens to match that for signals. The question is - why would you want a signal to auto-fire?

Perhaps you can emit null into a signal, which will do the job.

@suterma
Copy link
Author

suterma commented Oct 1, 2024

This specific event of mine is a "ready" event. It is only emitted once ever, the first time a resource is ready for interaction. So, if the subscriber subscribes at a later time, it will never get the ready event. I wanted NOT to be it a boolean, because it will never ever fall back to false, and this definition would suggest otherwise.

However, the variant with null worked perfectly for my purpose. Thanks for the hint!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants