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

rxdart 0.27.2 introduces a breaking change to BehaviorSubject.map #625

Open
ryanheise opened this issue Sep 9, 2021 · 7 comments
Open

Comments

@ryanheise
Copy link

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
Copy link
Collaborator

hoc081098 commented Sep 9, 2021

  • 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
Copy link
Author

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

@hoc081098
Copy link
Collaborator

That is method overriding, cannot deprecated them.

@ryanheise
Copy link
Author

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.

@alexgarbarev
Copy link

alexgarbarev commented Sep 16, 2021

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
Copy link
Collaborator

hoc081098 commented Sep 16, 2021

@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
Copy link

fdietze commented Oct 20, 2021

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.

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

4 participants