ReactiveX/rxdart

rxdart 0.27.2 introduces a breaking change to BehaviorSubject.map

ryanheise opened this issue · 7 comments

ryanheise commented

In rxdart 0.27.1, BehaviorSubject.map returns a ValueStream but this breaks in 0.27.2 which returns a Stream (a minor version increment). I like being able to map a ValueStream into another ValueStream, so maybe this interface should be defined as part of the ValueStream API, but in any case I think the behaviour should at least be brought back to BehaviorSubject to avoid the breaking change.

hoc081098 commented
  • That is just a internal workaround to keep behavior of BehaviorSubject (replay the latest value when listening) when using built-in operators.
  • That is an mistake to expose return type of built-in methods as ValueStream
  • Returned ValueStreams have not been intented for direct use, it causes many problems (because value is replayed asynchronously after a microtask)
Before:
var s = BehaviorSubject.seedee(1);
print(s.value); // 1

ValueStream<int> mapped = s.map((v) => v + 1);
print(mapped.value); // throws Error

Safety way is using shareValueSeeded(...)

Now:
var s = BehaviorSubject.seedee(1);
print(s.value); // 1

ValueStream<int> mapped = s.map((v) => v + 1).shareValueSeeded(s.value + 1);
print(mapped.value); // 2
ryanheise commented

That said, it is a breaking change with no prior indication that this was an internal method, so it breaks apps/plugins that were previously depending on it. I think the correct way to remove this method would be to first deprecate it and remove it in the next major version.

But I think a better solution is to improve the semantics of this method rather than remove it in the next release. i.e.

  • If the current value stream has a value, return shareValueSeeded
  • else return shareValue
  • 👍1
hoc081098 commented

That is method overriding, cannot deprecated them.

ryanheise commented

Ah, I didn't think of that. Although the solution I really prefer doesn't require deprecation anyway ;-) i.e. Keep the override so as to avoid any breaking change on a minor release, and then pave the pathway to support a specialised map for all ValueStreams with better semantics.

  • 👍1
alexgarbarev commented

Yeah, that change broke my app as well. I wasn't aware of all methods available in rxdart, and I read "changelog" after upgrade, but I couldn't figure out how to fix it, before I found this thread.

I believe that for this change, we should add a note in a "changelog", and suggest a workaround with correct syntax as mentioned in there #625 (comment)

Also, what would be a workaround for asyncMap? I used to use asyncMap from BehaviorSubject to load data, associated with value. I can't just load it every time I access value. I need latest value async mapped from BehaviorSubject. Or maybe I misunderstand argument of shareValueSeeded? - is it initial value, or it would be called every time I access .value getter?

btw thanks for the library overall :)

hoc081098 commented

@alexgarbarev

shareValueSeeded() returns a Stream that subscribe to source stream when the first listener subscribe to it

final s = BehaviorSubject.seeded(1);

Future<String> converter(int v) {  }

ValueStream<String?> mapped = s.asyncMap(converter)
     .cast<String?>()
     .shareValueSeeded(null); // null means loading state

String? value = mapped.valueOrNull; 
if (value == null) {} // loading
else {}

mapped.listen((String? value) {
   if (value == null) {} // loading
   else {}
});

Sent from my Redmi 7A using FastHub

fdietze commented

This change broke my app as well. I think it's ok to release breaking changes, as long as semantic versioning is followed.
See https://dart.dev/tools/pub/versioning for more details.

  • 👍1