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

RFC: Rename signal.changes to... signal.updates? #132

Open
raquo opened this issue Nov 4, 2024 · 4 comments
Open

RFC: Rename signal.changes to... signal.updates? #132

raquo opened this issue Nov 4, 2024 · 4 comments

Comments

@raquo
Copy link
Owner

raquo commented Nov 4, 2024

signal.changes is a stream that emits all events / updates in signal, EXCLUDING its current value ("current" at the moment of activating the subscription).

The name .changes implies that there is some deduplication going on under the hood, and in fact that was the case pre-15.0.0, when signals had built-in == checks. But such deduplication is not there anymore, so the name stopped serving us well.

I suggest keeping .changes available but @deprecated, since there are a bunch of materials online referencing it, and creating a new method called .updates that does the same thing. Feel free to suggest alternative names. Note that the name should work for several methods: signal.changes, signal.changes(_.distinct), signal.composeChanges(_.distinct).

Another potential naming option is .toStream, but this name does not reflect the fact that the initial value will be missing from the stream. That will surely surprise new users.

BTW If anyone wants to suggest a change in how the changes API should work, now would be a good time.

@yurique
Copy link
Contributor

yurique commented Nov 4, 2024

Another potential naming option is .toStream

When I started reading the post I immediately wanted to ask if there is a way we could have an actual .toStream which will include the initial value.

It's easy to implement in the "userland":

EventStream.merge(
  EventStream.unit().sample(signal),
  signal.changes
)

but maybe it could be implemented more efficiently.

I don't have a use-case at hand right now (I'll look for it) - for the signal-to-stream transformation - but I know I end up doing this every now and then :)

@raquo
Copy link
Owner Author

raquo commented Nov 4, 2024

Yes, good point, that's another reason not to use toStream name for changes – we may eventually need it for a more direct conversion.

So far I have resisted adding a real toStream as you described mostly because I didn't quite like its behaviour in regards to the current / initial value. Your code snippet is exactly how I would have implemented signal.toStream, but that implementation has an edge case: if that stream is stopped (loses all observers), and then re-started (obtains a new observer), it will re-emit the signal's current value. But, that's how signals behave, that's generally not how streams behave.

However, in recent versions of Airstream we have a special mechanism for re-starting streams that depend on signal, e.g. if you restarted signal.changes in the same manner as I described above, that stream would only emit the signal's current value when re-starting if the signal remained active while signal.changes was stopped (i.e. signal has other subscribers), AND has emitted a new (not necessarily distinct) value while signal.changes was stopped.

We could, I guess, implement a toStream method with such a mechanism as well, and it would behave as desired – pretty much like signal.changes except it would also emit the signal's initial value when the stream is first started (but not on re-starts, unless signal.toStream needs to catch up to events that it missed in signal, similar to .changes).

Would that be the desirable behaviour for you? Also, at a high level, what's your most common use cases for wanting .toStream? I was hoping that with signals behaving more like streams without the == checks, the need for toStream would be less.

But that still leaves us with the other issue: the signal's current value is not an event that is pushed – it is not fired in any transaction – it's effectively a value that is pulled – and by design that happens outside of any transaction, as transactions are not really needed when pulling. So, signal.toStream would need to fire that initial value in a new transaction (because there isn't necessarily another ongoing transaction to fire it in). And, depending on the use case, I think that could be problematic, for example if you wanted to convert a signal to a stream so that you can combineWith it with another stream, you could see a diamond case glitch if that other stream emits at the same time as your signal – the combined signal will emit two events in that case, instead of combining them into a single event.

But to be honest, I'm not sure if the transaction concern is real or just theoretical. What kind of observable graph / use case would see that problem? The problem is only for the initial value, not subsequent events. It would need to be something non-trivial to cause a problem I think. And, keep in mind that signal.changes suffers from the same problem, although that one is only a narrow edge case (re-starting signal.changes when signal has had updates while signal.changes was stopped), so most users would have no chance of running into that.

So, .toStream is definitely on my radar, but I'm not sure about it yet – it would be a very inviting method for beginners to use, and so it needs to be predictable and not prone to glitches.

@ivan-klass
Copy link

ivan-klass commented Nov 30, 2024

I would also love this feature to exist. Currently what I've done locally is

  extension [T](signal: Signal[T])
    inline def toStream: EventStream[T] = new StreamFromSignal(parent = signal, changesOnly = false)

@raquo
Copy link
Owner Author

raquo commented Nov 30, 2024

@ivan-klass This may not work in all edge cases. See #124

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

No branches or pull requests

3 participants