Comments (7)
The problem here is that asBroadcastStream()
is indeed using the asBroadcastStream
of the wrapped stream, which uses the listen
of the wrapped stream.
I agree that the DelegatingStream
should not depend on StreamView
. The two classes have different purposes. The StreamView
is there to hide methods that are not on Stream
(for classes that implement Stream
and something more, which is bad idea). The DelegatingStream
intends to work like the wrapped stream, but allows you to override specific methods.
For a Stream
, the only method that really matters is listen
(and the isBroadcastStream
getter to some extent). Everything else is defined in terms of that, in a very consistent way.
So, if some class overrides asBroadcastStream
, what is the correct behavior?
- Should
DelegatingStream
use the overriddenasBroadcastStream
? - Should
DelegatingStream
ignore the overriddenasBroadcastStream
and use the default behavior?
Currently we are doing the former. That bites us here because the overridden asBroadcastStream
has no way of seeing the DelegatingStream
wrapper.
On the other hand, it's a weird "delegating" stream wrapper that doesn't actually use the stream it wraps.
On the third hand (I wish!), that's what we do for all the other methods, they just use the default implementation based on listen
, even though the wrapped stream might override last
more efficiently.
No clear and good solution here. There is a consistency argument for not forwarding asBroadcastStream
to the underlying stream, and a practical argument that nobody overrides asBroadcastStream
in practice (and those who do, they do it so badly that they are better ignored).
Changing the behavior is theoretically a breaking change - if someone wraps a stream which overrides asBroadcastStream
and depends on the overridden method to be called, changing it will break that code.
All in all, I'm not sure the risk of breakage, no matter how hypothetical, is worth it.
I think the best solution would be for DelegatingStream
to stop extending StreamView
, and just do the listen
/isBroadcastStream
forwarding itself.
from async.
I'm not talking about overriding asBroadcastStream
, I just want to override listen
, however I would expect asBroadcastStream
to subscribe to the DelegatingStream
subclass, not directly to the wrapped stream. I.e., I would expect this implementation:
class DelegatingStream<T> extends Stream<T> {
@override
bool isBroadcastStream => _delegate.isBroadcastStream;
@override
StreamSubscription<T> listen(...) =>
_delegate.listen(...);
final Stream<T> _delegate;
DelegatingStream(this._delegate);
}
I realise it is technically a breaking change, but I think it is rare for developers to assume DelegatingStream
extends StreamView
, so changing that shouldn't be an issue. It's a weird thing for the class to be doing anyway. I think it is more likely that this problem hasn't been noticed before because asBroadcastStream
is not used a lot, which really means every DelegatingStream
is a ticking time-bomb just waiting for someone to call asBroadcastStream
.
from async.
I can understand that expectation, but it is not what other DelegatingX
classes do. Instead they delegate every function to the wrapped object.
If DelegatingStream
did the same thing, then overriding listen
would not change anything except direct calls to listen
, all other methods would forward to the wrapped stream's methods which uses the wrapped stream's listen
.
So, should DelegatingStream
be consistent with other DelegatingX
classes, or should it do what you expect? That depends entirely on the use-case.
With the "only forward listen
" approach, you can't meaningfully override any other method. If you do, it might not be called anyway, because the receiver might also wrap your stream and only forward the listen
.
In practice we might two different classes, one that only forwards Listen
and one that forwards everything.
Or, since only forwarding listen
means that there is only one function to care about, we could introduce a constructor like Stream(StreamSubscription<T> onListen(), {bool isBroadcast = false}) => _OnListenStream<T>(onListen, isBroadcast);
, with a supporting class of:
class _OnListenStream<T> extends Stream<T> {
StreamSubscription<T> Function(bool cancelOnError) _onListen;
final bool isBroadcast;
_OnListenStream(this._onListen, this.isBroadcast);
StreamSubscription<T> listen(
void onData(T data), { Function onError, void onDone(), bool cancelOnError = false }) {
var sub = _onListen(cancelOnError);
if (onData != null) sub.onData(onData);
if (onError != null) sub.onError(onError);
if (onDone != null) sub.onDone(onDone);
return sub;
}
}
(Not saying that we are in any way consistent now, forwarding asBroadcastStream
and nothing else, but I do think that the change should be made to DelegatingStream
, not StreamView
).
from async.
I see, that makes sense. My assumption would be that most people who want to wrap a stream would want all the methods to go via the overridden listen
, but your point of other classes named Delegating
forwarding all calls makes sense too.
It's pretty straightforward to just extend Stream
and override the two methods myself, so this isn't a big issue for me, but I think we're in agreement that DelegatingStream
is not implemented correctly, whatever "correct" means in this case.
from async.
@lrhn – is there an SDK issue here?
from async.
I don't think we will change the SDK classes. The DelegatingStream
from package:async
may want to change to either forward all members, or not forward asBroadcastStream
(or having both versions available).
from async.
Transferred the issue
from async.
Related Issues (20)
- Change the default of `propagateCancel` argument in CancelableOperation.then HOT 2
- Reset method for AsyncMemoizer HOT 1
- Make it easier to safely hold a reference that can cancel an operation without holding the whole operation HOT 1
- Clarify `StreamQueue.next` will fail just after `hasNext` in API document.
- Consider to make second invocation of `streamQueue.hasNext` be postponed concluding the result until the first invocation of `q.next` , unless the stream is closed. HOT 6
- Deprecate StreamQueue.hasNext and StreamQueue.next
- Future.wait() but with Records HOT 4
- Bad State error while trying to reject a StreamQueueTransaction
- Dart 3 incompatibilty: `DelegatingStream<T> extends StreamView` but `StreamView` is `base class` HOT 5
- Add `whereNotNull` for `Stream`
- CancelableOperation value is not propagating errors, so they cannot be catched and app is crashing HOT 3
- There should be cancellable versions of Stream.firstWhere etc.
- Migrate the Result type to sealed classes HOT 2
- Make `ParallelWaitError` Include Error Details HOT 1
- Async Cache is caching exceptions HOT 5
- AsyncMemoizer is caching exceptions HOT 2
- Add an API wrapping runZonedGuarded that surfaces the first error HOT 4
- Clarify `CancelableOperation` docs HOT 4
- Inconsistent behavior of `Stream.listen` on broadcast streams HOT 1
- [Proposal] Add a CountDownLatch implementation
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
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.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from async.