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
Whats the motivation for creating ValueWrapper and breaking backwards compatibility with ValueStream? #556
Comments
It is necessary when dealing with nullable type. Trying import 'rxdart', old code Sent from my Redmi 7A using FastHub |
Why would it ever be necessary to seed a |
Sure, but I don't see how that is related to creating ValueWrapper? Does ValueWrapper fix #170? I don't see a problem being fixed with ValueWrapper, only moved. Correct me if I'm wrong on that. |
Sent from my Redmi 7A using FastHub |
But it seems to me you're doing a hacking workaround. I'm looking at the code: rxdart/lib/src/streams/value_stream.dart Line 23 in 898d46a
Why is T being forced as nullable here? That doesn't really make sense, as it's nullability status should be set by the invoker, not the library. Maybe the invoker knows that the type is non-nullable; if so, it doesn't make sense that you're forcing it nullable here. Here's a small example of what I mean: class SimpleValueStream<T> {
SimpleValueStream(this.val);
final T val;
}
void main() {
final test = SimpleValueStream<num?>(null);
print(test.val); // Works fine
final test2 = SimpleValueStream<num>(null);
print(test2.val); // Will fail with: `The argument type 'Null' can't be assigned to the parameter type 'num'.`
} Now in my simple example above, T will inherits it's nullability from the invoker And thats my point, the type should be external - nullable or not, and rxdart should not, for even one second, care what the type is. If a test is failing, then rewrite that test. Don't introduce concepts like wrappers whose sole purpose is fixing a test. Fix the test instead. :) |
Yeah we need to roll this back, value should just match the Stream's type, always. @hoc081098 if seeded(null) on T? Is problematic, maybe we can solve it differently? I'll try to take a look tomorrow. |
What should be returned when ValueStream has no data? From prev version, it returns |
You can use |
But that is not really a clean solution either. As I mentioned above, it should be possible without all these strange workarounds. |
Yeah that's basically the root of the problem, in fact, What if we get rid of the wrapper, and just offer
then: we throw a special Error, e.g. class UnsyncedValueError<T> implements Exception {
final Future<T> valueFuture;
String get message =>
'''A request for ValueStream.value was made, but no value has yet been emitted from the Stream.
Alternatively, wait for the value to be emitted by using UnsyncedValueError.valueFuture in a try/catch block.''';
const UnsyncedValueError(this.valueFuture);
}
void main() async {
final subject = BehaviorSubject<int>();
try {
print(subject.value);
} on UnsyncedValueError catch (e) {
print(await e.valueFuture);
}
} but, it is possible that the |
Let me bring some perspective, I know how it can be difficult to break out of a certain mental mindset. When seeded, obviously the value is non-nullable, because you supply a value (unless you seed with a nullable type ofc, which is perfectly acceptable as the type is inherited anyway). If not seeded, and you just use the constructor, the type is forced nullable, as it should be. It would look something like this:
Behind the scenes, it would look something like this: class SimpleBehaviorSubject<T> {
T _val;
T get value => _val;
SimpleBehaviorSubject(this._val);
/// Can be nullable or not, BehaviorSubject is unopinionated about it.
factory SimpleBehaviorSubject.seeded(T val) {
return SimpleBehaviorSubject(val);
}
}
void main() {
final unseeded = SimpleBehaviorSubject<num?>(null);
print(unseeded.value);
final seeded = SimpleBehaviorSubject.seeded(1);
print(seeded.value);
} Again, the main goal should be to strip all Rxdart bias towards a variable being nullable or not. It should mind it's own business and do what it does best: streams. 😁 |
@larssn I don't think it's acceptable to just change to T? when non-seeded :) The example you provide, also won't compile, as a factory ctr cannot just return a changed Type. We're just trying to solve the problem of value, that's all, which actually can be seen as |
Sorry I'm in Typescript mode most days. I probably should have run it through a compiler first 😇 Anyway, I've updated my example above to something that works. The only quirk is explicitly defining that the unseeded version takes "null" as a param. Not sure if dart can figure out that the type is external, and should not be inferred from within the class itself.
You have to! There literally might never be a value. It's the only sound way. |
The only sound way is to keep the expected Type! If there never will be a value, and you are trying to access value, then IMO, an error is the only reasonable way forward,
Now, that maybe could be done differently, but please not by just changing the Stream's type to nullable. |
I agree with you: The type should be what the user defines - which is exactly what my revised example above does. If the programmer using rxdart defines the type to be non-nullable, and then later feeds nulls into the stream, he'll get a crash: Dart already handles that. Rxdart does not need to. Anyway, before we get off track. My main point opening this thread was simple this: ValueWrapper solves nothing - it only moves a problem that should not be the responsibility of rxdart in the first place. So I questioned it's existence. |
@hoc081098 @larssn see #559 This PR is just a proposal, it brings back It's perhaps not too different from |
Let me add my two cents: I think creating the ValueStreamExtensions might make it very difficult for newbies to work with RxDart. I know this is not your fault, but IDEs do not suggest "value" as a valid property unless you specifically import RxDart (because it's in an extension). I tend do use value to provide the initialData in StreamBuilders - in views, you most likely don't need to import RxDart (most RxDart code ends up in BloCs/ViewModels, etc.). People with less Rx experience might not know about it, and think: oh, ok, so no more "value". This makes features discovery very difficult. I'm also a bit frustrated that I need to rewrite all my .value = code to .add(). |
@lukaszciastko that's true, I'd prefer just a Introducing nnbd was no easy feat, and many thanks to @hoc081098 for pushing that forward. |
#560 will add setter |
Let me start by saying say that the work done on this library is awesome, thank you to all the contributors, you make our life with dart streams a lot easier 🙂 Now on this issue: I just stumbled upon this when trying to upgrade to the latest RxDart version. I don't really understand the need to rename the field Even before nnbd, the contract of the I'm not sure I like the new Why not just keep the contract simple with just a |
final s = BehaviorSubject<int?>.seeded(null);
print(s.valueOrNull); // prints `null`
print(s.requireValue); // prints `null`
print(s.valueOrNull!); // crash :)) |
Yes, it's not the same in case of nullable type, but it makes not much sense to call |
I still don't understand why rxdart embraces all these kinds of validity checks. Would have left it up to the consumer of the library and kept the fields to a minimum, if it was me. I don't need the library to hold my hand when it comes to type checks. But I guess I don't have to use them. 🙂 |
I see you went with renaming final countStream = BehaviorSubject<int>();
print('count is ${countStream.value ?? 'unknown'}'); will now crash without much warning if the edit: messed up the PR reference |
Null safety isn't perfect, and this an example of something that is clearly annoying. But the pros outweigh the cons IMO. |
Maybe splitting BehaviorSubject into two separate classes would have been better. There are certain limitations in the dart type system that makes it difficult to implement a version of the same class that is sometimes nullable and sometimes not. Example: I'm not fond of that approach either tbh, but it seems implementing this correctly isn't possible, and all the weird "helper" fields that keeps getting added, points to that symptom. |
Why? I think it makes sense. |
Idd, there's a lot of focus on value, but the main purpose is to listen, starting with the latest value as first event. Imo, it's perfectly fine to throw a null safe error when accessing value too early, imo the only correct type of value would be FutureOr, but bottom line it's inherently unsafe to trust value, devs should always be careful when relying on it. |
The point of NNBD is to give more compile time safety and to avoid null safe error. Dart now gives you language constructs and flow analysis to make sure that doesn't happen. I agree with @larssn that having 2 versions of BehaviorSubject (seeded and unseeded) might be a cleaner solution but it's not realistic I think. Anyway, as long as there is the |
List.first is also unsafe, isNotEmpty kinda is like hasValue |
I want to clarify that from a design perspective, I agree that having the "nullability" of I understand the analogy with List.first, but List.first has always thrown, even before NNBD. |
Let me elaborate. Since we're discussing Now the STREAM should not emit null unless the type was nullable: <int?>. But that is a trivial problem to solve. In any case, I think we agree on a fundamental level. |
I think it depends how you define |
|
As long as you are aware of this, I'm cool with it 😄 |
If I'm following then you are close to describing v0.25, with @hoc081098 I love kotlin, it's great. How would that look in practise? Something like this?
Because I don't think we need a new exception, as we have one built in if we're trying to force a null value into a non-nullable field. EDIT:
|
I'm not sure I understand the problem 😅. class Wrapper<T> {
T? value;
Wrapper([this.value]);
}
void main() {
var wrapper1 = Wrapper<int>();
var wrapper2 = Wrapper<int?>();
print(wrapper1.value); // value is of type int? and prints 'null'
print(wrapper2.value); // value is of type int? and prints 'null'
} |
We're trying to solve value being non-nullable in some situations (I think... ARE we still trying to solve that? 😁). Your example works, but it is what Petrus tried to solve with the ValueWrapper that prompted me to start this thread - namely that value SHOULD BE non-nullable in certain situations (like with a seeded BehaviorSubject). But it's clearly not possible. All this discussion and we've gotten nowhere except more fields that does little more than cast a value to non-null (something the consumer can do himself). So actually I'm fine with the simplest of all implementations: Yes I don't think it's worth the trouble to implement all these "valueOrNull, or requiredValue, etc"-fields. All that work to bypass a simple null check. |
Well OK, that was my counter-point : the value exposed in
So yes, we agree 😄 |
If we have only one Then, bool hasValue(ValueStream<T> valueStream) {
try { valueStream.value; return true }
on ValueStreamError catch(_) { return false; }
} and T? valueOrNull(ValueStream<T> valueStream) {
try { return valueStream.value; }
on ValueStreamError catch(_) { return null; }
} We can move it to |
@frankpepermans @jyardin @larssn |
I am upgrading to the latest version and I get an error on this line There isn’t a setter named 'value' in class 'ValueStreamExtensions'. |
ValueWrapper
seems to do nothing other than wrap another value, whichValueStream
also did.So why create it? Was it really necessary to break everyone's code due to this seemingly useless class? I'm genuinely curious as to why this was added.
So our code is going from
_valueStream.value
to_valueStream.valueWrapper.value
.The text was updated successfully, but these errors were encountered: