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

Switch to SingleSubscriptionTransformer<S> #95

Open
natebosch opened this issue Jan 14, 2020 · 4 comments
Open

Switch to SingleSubscriptionTransformer<S> #95

natebosch opened this issue Jan 14, 2020 · 4 comments
Labels
next-breaking-release Issues that are worth doing but need to wait for a breaking version bump

Comments

@natebosch
Copy link
Member

natebosch commented Jan 14, 2020

Currently its SingleSubscriptionTransformer<S,T>, but the purpose is not to allow changing the generic on the stream type. See TODO comment on in the file.

@natebosch natebosch added the next-breaking-release Issues that are worth doing but need to wait for a breaking version bump label Jan 14, 2020
@lrhn
Copy link
Member

lrhn commented Jun 10, 2020

Not clear what should be changed. Can you elaborate?

@natebosch natebosch changed the title Switch to SingleSubscriptionStream<S,S> Switch to SingleSubscriptionTransformer<S,S> Jun 11, 2020
@natebosch
Copy link
Member Author

natebosch commented Jun 11, 2020

Sorry, I mistyped the name.

This is a tracking issue for:

// TODO(nweiz): When we release a new major version, get rid of the second
// type parameter and avoid this conversion.

This was never intended to be used in any way other than the case where both generics are the same.

Concretely, this class should be defined as:

/// A transformer that converts a broadcast stream into a single-subscription
/// stream.
///
/// This buffers the broadcast stream's events, which means that it starts
/// listening to a stream as soon as it's bound.
class SingleSubscriptionTransformer<S> extends StreamTransformerBase<S, S> {
  const SingleSubscriptionTransformer();

  @override
  Stream<S> bind(Stream<S> stream) {
    StreamSubscription<S> subscription;
    var controller =
        StreamController<S>(sync: true, onCancel: () => subscription.cancel());
    subscription = stream.listen(controller.add,
        onError: controller.addError, onDone: controller.close);
    return controller.stream;
  }
}

@natebosch natebosch changed the title Switch to SingleSubscriptionTransformer<S,S> Switch to SingleSubscriptionTransformer<S> Jun 11, 2020
@lrhn
Copy link
Member

lrhn commented Jun 12, 2020

Ack.

FWIW, I'd implement it as:

  @override
  Stream<S> bind(Stream<S> stream) {
    var controller = StreamController<S>(sync: true);
    controller.addStream(stream).whenComplete(controller.close);
    return controller.stream;
  }

(I also think it's a bad idea to listen immediately and buffer an unboundend number of events, but if that's the desired semantics, I guess it has to happen).

Let's make the Null Safe release a new major version, that should let us fix things like this.

@natebosch
Copy link
Member Author

Let's make the Null Safe release a new major version, that should let us fix things like this.

We really hope to be able to do that 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-breaking-release Issues that are worth doing but need to wait for a breaking version bump
Projects
None yet
Development

No branches or pull requests

2 participants