Testing of BehaviorSubject with seed or first `null` value

[lwasyl]

I can't seem to test properly that BehaviorSubject emits null value first, either with add method or with seed value. I'm not sure if this is I'm doing wrong, but here's what I have:

test("Can expect initial non-null value", () {
  final subject = BehaviorSubject<String>(seedValue: "value");

  expect(subject, emits("value"));

  subject.close();
});

test("Can expect initial null value", () {
  final subject = BehaviorSubject<String>(seedValue: null);

  expect(subject, emits(null));

  subject.close();
});

For the non-null value the test passes. For the null value, the assertion fails because the stream is closed. If the stream is not closed, the test timeouts.

I feel something is off with null values being pushed as initial or first values to the subject, for example this:

test("Print initial values", () {
  BehaviorSubject<String>(seedValue: null)
    ..listen(print)
    ..add(null)
    ..add(null)
    ..close();
});

will print null only twice. When I replace nulls with instances, the value is printed three times.

Overall seems like the existing null value isn't pushed to new subscribers. I haven't found it documented that for null it'd behave differently, and also I haven't found any information about special handling of null for RxDart in general.

[frankpepermans]

The default value of the seedValue is in fact null, so writing BehaviorSubject(seed: null) is exactly the same as BehaviorSubject().

But you are right, as null is a valid event, so we should support it as a seedValue too :)

I'm thinking we could change the default value to an inner class instance, const _SeedValue() for example, which we can then check against.

[marty331]

BehaviorSubjects by default need an initial value, I don't believe setting a null valid is a valid use.

[lwasyl]

BehaviorSubjects by default need an initial value

I don't think they do -- if not provided one they'll simply not emit anything until some value is pushed. Am I wrong?

I don't believe setting a null valid is a valid use

It is valid for me -- since I'm allowed to emit null values, I'd expect them to be treated the same way regardless of position in the stream. I can wrap my emissions as a workaround, but then I'd either prefer null values to be forbidden like in RxJava2, or BehaviorSubject to be properly documented

[lwasyl]

@frankpepermans Seems like the initial value is already wrapped in an inner class, and that doesn't solve the problem, because the constructor can't know if null value was provided, or no parameter was passed. I think SeedValue would need to be public for BH to be able to distinguish that

[frankpepermans]

Yeah I was a bit too fast there, seedValue is of generic Type T, so we can't really default it to some inner class.

Another solution might be to use T seedValue() instead, but that's somewhat annoying i.e. new BehaviorSubject(seedValue: () => null).

The default value would still be just null, so if no seedValue is passed, nothing happens, if it is a method like above, we add the resolved value.

[brianegan]

Hrm, this is an interesting problem in general... It actually goes beyond the seedValue as well. For example, this won't work either:

test('subject null values', () {
  final subject = new BehaviorSubject<int>(seedValue: 1);
  // Receives 1, then null. This passes.
  expect(subject, emitsInOrder(<dynamic>[1, null, emitsDone]));

  // Add a null value, replacing the seed
  subject.add(null);

  // Fails because it never receives null
  expect(subject, emitsInOrder(<dynamic>[null, emitsDone]));

  subject.close();
});

However, if we start allowing nulls by default, which would fix both the original issue and the problem in the test above, everyone who currently uses BehaviorSubject without a seedValue would start seeing nulls emitted as the first item, and their code might not be ready for that (and I also think it's kind of weird).

Another option: create a new factory, such as BehaviorSubject.replayNull() with the same options as the normal constructor. However, if the seedValue is null or the last emitted value was null, it would replay it. This would fix both of these cases, and allow folks who want to use null to do their thing

[lwasyl]

The following diff seems to fix both cases -- initial and later emissions -- and is backwards-compatible. Alternative would be a different factory which implies replayNulls. I ran all tests in project, but maybe there's another case that isn't covered? I only added the two test cases for initial null emission and the one you pasted, both with replayNulls: true.

--- a/lib/src/subjects/behavior_subject.dart
+++ b/lib/src/subjects/behavior_subject.dart
@@ -50,6 +50,7 @@ class BehaviorSubject<T> extends Subject<T> {
     void onListen(),
     Future<dynamic> onCancel(),
     bool sync: false,
+    bool replayNulls: false
   }) {
     // ignore: close_sinks
     final StreamController<T> controller = new StreamController<T>.broadcast(
@@ -63,7 +64,7 @@ class BehaviorSubject<T> extends Subject<T> {
     return new BehaviorSubject<T>._(
         controller,
         new Observable<T>.defer(
-            () => wrapper.latestValue == null
+            () => !replayNulls && wrapper.latestValue == null
                 ? controller.stream
                 : new Observable<T>(controller.stream)
                     .startWith(wrapper.latestValue),

[frankpepermans]

I'd say we keep it simple, drop seedValue from the default contructor, and present a new one, BehaviorSubject.seeded(T value).

If the latter is called, we can skip the initial null check and just add the value as the first event.

Thoughts @brianegan?

[frankpepermans]

As a follow up, BehaviorSubject has a sync value getter, now it will yield null if no even is added and no seedValue was set.

I'd prefer it to throw an Exception instead in this case though, in the lines of List.first, which throws when the List is empty

[brianegan]

@frankpepermans Yah, so many issues to consider!

I'd say we keep it simple, drop seedValue from the default contructor, and present a new one, BehaviorSubject.seeded(T value).

I think the problem is that the initial null check not only applies to seed values, but any emitted nulls as well (what I demonstrated in the test case above). Therefore, it's really about allowing null values to be replayed in general, unless we want to distinguish somehow between the seedValue and the values emitted later.

However, that could lead to complex / perhaps unexpected results (I could see someone thinking: why is it returning null after I emit something, but not as the seedValue?). This leads me to favoring a solution like the one @lwasyl has proposed.

As a follow up, BehaviorSubject has a sync value getter, now it will yield null if no even is added and no seedValue was set.

I think this suffers from the same problem as the point above. In this case, I think we'd need to add isNotEmpty semantics as well to the Subject, since this is how I avoid exceptions when I use Iterable.first myself, e.g. myList.isNotEmpty ? myList.first : 'defaultValue'. Maybe I'm crazy, but I don't mind value being nullable in some ways, since you can just do: subject.value ?? 'defaultValue. Even then, it seems like ya should just set the seedValue so you don't have to do that logic everywhere you consume the value property.

After thinking about it a bit, I kinda think what we have is pretty sane as the default (assuming folks won't emit null), and we should offer a specific constructor option for peeps who want to use null for seedValues or as emitted values.

What do you all think? Dang null values! Always a tricky one, haha :)

[frankpepermans]

I think having a special seeded constructor works, just keep a flag "isEmpty" which is defaulted to true for the plain ctr, and false for the seeded one.

When calling "listen" or "value" you then have the flag to disambiguate the null value.

[zimmi]

I hit this as well today.

After thinking about it a bit, I kinda think what we have is pretty sane as the default (assuming folks won't emit null), and we should offer a specific constructor option for peeps who want to use null for seedValues or as emitted values.

I agree, the specific constructor route makes the most sense. That way we can keep backward compatibility.

Reading this issue, I made a table with the behavior that I think is useful / should be supported and the way the current suggestions address them:

	today	@lwasyl 's suggestion	@frankpepermans ' suggestion
1 replay nulls + null seedValue	no	yes	yes
2 replay nulls + no seedValue	no	no (will use null seedValue)	yes
3 replay nulls + nonnull seedValue	no	yes	yes
4 isEmpty support	no	no	yes
5 backward compatible	yes	yes	breaks users of seedValue in default constructor, breaks users of default constructor immediately accessing value

Any objections to these goals?

To make a case for 4: When it's not possible to provide a seedValue, having the isEmpty / isNotEmpty getters is useful information that is currently not available. Assuming the new constructor that allows emitting null is used and not specifying a seedValue, getting null from value makes it indistinguishable from a null seedValue.

---

P.S.: If we provide an alternative constructor that supports nulls, it might make sense then to disallow nulls completely for the default constructor. Right now the behavior is inconsistent and programs relying on emitting nulls are likely to be subtly broken (no replay of nulls).

[alextekartik]

Since I ran into this as well, I allow myself to give my opinion. I consider the fact that null are not emitted as a bug i.e. the first test below fails (and looking at it it should obvisoulsy succeeds) and the other one succeeds. Nothing in the documentation of BehaviorSubject talks about null value not supported.

test('behavior_subject', () async {
  var subject = BehaviorSubject();
  subject.add(null);
  // Fails!
  await expectLater(subject.stream, emits(null));
});

test('stream', () async {
  var controller = StreamController();
  controller.add(null);
  await expectLater(controller.stream, emits(null));
});

so I'd rather fix the bug and break the compatibility (yes not everybody will agree on that) and emit nulls (even when starting listening). I ran into this in both my flutter (and in stream builder people are used to handle the initial null value) and in angular (was wondering why sometimes I received the initial seed data and sometimes not) so I ended up writing my own class to supports that (not based on rxdart), too bad as I consider BehaviorSubject the best solution in a BLoC architecture (especially the ability to get the last value instead of only Stream and Sink)

For users that do not want null value we could consider adding .nonNull() transformer like we have a .distinct()

Otherwise, thanks for this nice package!

[lwasyl]

Alexandre reminded me of this issue, so I attempted to fix it in https://github.com/ReactiveX/rxdart/pull/195. @brianegan @frankpepermans Let me know what you think about the PR, specifically changes in ValueConnectableObservable and Observable itself. Seemed reasonable to expand the API to allow the same customization as in BehaviorSubject itself

Those pesky null values :\

[alextekartik]

Actually I was wrong in my statements. null values are emitted (as mentionned in the other comments) except the first one. Below is a simple test where it looks definitely wrong

import 'dart:async';
import 'package:test/test.dart';
import 'package:rxdart/rxdart.dart';

main() {
  testBehaviorSubject(List<int> values) async {
    // Set the initial value from the first item in the array
    var subject = BehaviorSubject<int>(seedValue: values.first, sync: true);
    List<int> emittedValues = [];
    var subscription = subject.stream.listen((int value) {
      emittedValues.add(value);
    });
    // add the other values
    for (int i = 1; i < values.length; i++) {
      subject.add(values[i]);
    }
    // Dummy delay, it seems sync is not working properly...
    await Future.delayed(new Duration());
    expect(emittedValues, values);
    await subscription.cancel();
  }

  test('behavior_subject', () async {
    await testBehaviorSubject([1, 2, 3]);
    await testBehaviorSubject([1, 2, null]);
    await testBehaviorSubject([1, null, 3]);
    // Only this one below fails...
    await testBehaviorSubject([null, 2, 3]);
  });
}

as a side not it seems the sync flag is not working properly.

[tomwyr]

As a workaround you can also use ReplaySubject and prepend it with start value:

Subject subject = ReplaySubject(maxSize: 1)..add(null);

[lukepighetti]

Hi everyone! Been running into this recently. seedValue: null should emit a null event on listen of a BehaviorSubject in my personal opinion. The ..add(null) method does not work for me with BehaviorSubjects for some reason. My only solution has been to emit a known incorrect value and then filter it by the subject consumer, which is not ideal. Thanks for listening! (harhar)

[Jonas-Sander]

Is there any status on this? #195 seems to be dead since two months?

[brianegan]

Hey all, appreciate your patience as it's been a busy time for us.

Overall, this may be controversial, but after reading these comments I still think of emitting null values is an anti-pattern. In fact -- RxJava does not even allow it! I really appreciate the contribution from #195, but overall it changes the default constructor of BehaviorSubject, and I think we should have a really high bar for changing the default constructor since BehaviorSubject is used a lot. Frankly, I don't think supporting null values meets that threshhold.

Could folks talk a bit more about why they need null and why would you expect a BehaviorSubject to emit null by default? That will simply require you to have a lot more null checking throughout your code, resulting in a lot of "defensive programming" -- and if you forget to do that, your users will run into an NPE.

If you want to emit nulls as an easy way to create a Stream that acts as a "signal" to perform some other work, you can always create a Dummy object and emit those instead.

Thanks all -- the reason this has been hard for me to pull the trigger on is, because frankly, I think using null is an antipattern and don't want it to creep into this library too much.

[lwasyl]

@brianegan no worries about the PR, I'm all for discarding what I wrote in favor of not permitting nulls. RxJava2 works that way and I think the community is OK with it, overall. I'm happy to contribute some changes if the decision is made.

[tomwyr]

@brianegan Thanks for this comment, it clarifies things a lot! I appreciate you're trying to ditch nullable types to avoid NPEs and really hope to see Dart supporting null-safe types sooner or later.

However, I believe that the issue lies in inconsistency of BehaviorSubject. As you pointed it out, RxJava refuses to emit any nulls, while in RxDart you can actually do that - it's just not possible to emit them as initial value. Actually, I spent like an hour trying to find out why this happens, until I discovered this issue on GitHub. I'd either expect subjects to completely forbid null values, or to point this behavior out in the documentation of BehaviorSubject.

[brianegan]

Yep, definitely! My proposal: I think we should add an assert statement or throw an ArgumentError if users attempt to add a null value to a Subject. That way it fails, but "fails fast" for end-users.

Completely agree the current behavior is ambiguous and we should provide good feedback for this case!

[frankpepermans]

@brianegan While I definitely agree on the null stance, I wonder if we should not just bite the bullet here.

Regular StreamControllers do allow to emit null Using shareValue could be problematic, if the original Stream does emit null

If we do allow null here, then I'd prefer a named ctr, so we'd have: BehaviorSubject() and BehaviorSubject.seeded(T event)

In the seeded version, we can just play whatever value event holds here. This is an acceptable refactor if you've used BehaviorSubject({T seedValue}) before.

[lwasyl]

@frankpepermans shareValue issue disappears if nulls are forbidden in the entire library and every rx stream, does it?

Regular StreamControllers do allow to emit null

I'd say that should be a feature and added value of RxDart that the streams won't be emitting nulls. Perhaps when wrapping a Dart stream to be an Rx stream, user could specify what to do with null values -- blow up, ignore, or map into some default?

I'll just mention that similar issue exists in RxJava, where you have plenty of places an external library could emit a null (when creating an observable, with subjects etc). Since in RxJava this behavior is an error, it is treated as such

[frankpepermans]

The difference to rxjavais, we piggyback on the existing Dart Streams. This is an ongoing struggle, because it does not allow us to closely follow the Rx spec.

Since Dart Streams are part of the core, I fully support that decision, but I do think we should then try to follow the choices Dart Streams made closely as well.

So if they allow to emit null, I think we should support it as well.

Consider a dev trying out Dart and rxdart:

• learn that rxdartis not a strict implementation, but build on Dart Streams
• learn Dart Streams and what is different to Rx
• come back, start using rxdart
• wait, rxdartdoes some things differently to Dart Streams after all, here's a list

[brianegan]

It's a completely fair point, and we have made the decision to favor Dart Streams over Rx spec when they clash. I think what's hard in this case is lining up Dart Streams with an Rx feature that takes for granted null values will not be emitted.

All that said -- I wonder if there's any way to stay in line with Dart Streams without requiring our entire community to refactor their code? I know it's not a big refactor, but it still feels like making this change will create work for a lot of peeps.

[frankpepermans]

Don't think we can, there used to be a check to verify that a Function parameter was passed, i.e. ?seedValue, but this was rolled back.

There's no arguments equivalent to say JavaScript either.

So having seedValue as an optional parameter is not a solution we can keep :/

[lukepighetti]

I think that you guys should pick a stance and stick with it. Can we emit null values through an Observable/Subject or not? If no, assert it. If yes, support it. To be frank, I don't really care which direction you decide to go.

I happen to leverage null in my code because all types in Dart are nullable. It's really that simple. I let the language dictate what is an anti-pattern. In languages with non-nullable types I don't do this. My only reason for supporting emitting null events is because in Dart all types are nullable. It sounds like this might change in Dart 3, but I'm not sure if this has been confirmed.

In RXJS (typescript) seedValue is required and should be required moving forward if we cannot emit null events. I would urge contributors to really consider the implications of this as I'm sure you will!

Can't wait to find out what you all decide.

[brianegan]

I would urge contributors to really consider the implications of this as I'm sure you will!

Yah, I think that's why it's been hard to pull the trigger on this one! Basically, it comes down to whether or not we want to break everyone's code to support this change, and I think there are compelling arguments on both sides.

However, at the end of the day, the core Stream library supports them, and there's just no way around it. When it comes to API conflicts, we've always chosen Dart over Rx, and Frank's point is right on the money when it comes to confusing folks.

Therefore, I think we just need to bite the bullet as much as it pains me, ask the community to refactor their seeded subjects, and deal with non-nullability if / when it comes.

How does that sound to folks?

[lukepighetti]

To be perfectly honest, refactoring for something like that is pretty trivial in my personal opinion. So I say go for it.