rrousselGit / flutter_hooks

React hooks for Flutter. Hooks are a new kind of object that manages a Widget life-cycles. They are used to increase code sharing between widgets and as a complete replacement for StatefulWidget.
MIT License
3.07k stars 175 forks source link

Confusing behavior of Stream hooks with default preserveState=true behavior #312

Closed rjpower closed 1 year ago

rjpower commented 2 years ago

Thanks for creating this library!

This isn't strictly a bug, but the preserveState=true default for useStream can lead to some confusing/inconsistent behavior. I observed this with an application where you have a parent stream that indicates transitions between application states, along with a child stream to show the underlying status:

class ChildEventType {}
class ChildEventA extends ChildEventType {}
class ChildEventB extends ChildEventType{}

enum AppState { a, b }

class AppEvent {
  AppState state;
  Stream<ChildEventType> childEvent;
}

Stream<AppEvent> appStream();

Expected behavior

I expect that should be impossible for an AppEvent to yield events which are inconsistent with the AppState, but the preserveState behavior on the stream gives this behavior.

You can see this behavior in the DartPad here: https://dartpad.dev/?id=02aa39ef7799d520d4f6e7c3d332da23

I suspect "I'm holding it wrong", and there's a more correct way to do this, but having this as the default made it hard to track down. (I'm filing this partly in case others run into the same issue).

rrousselGit commented 2 years ago

You're free to change that behavior if you want:

AsyncSnapshot<T> useAppStream<T>(Stream<T> stream, {bool preserveState = false}) {
  return useStream<T>(stream, preserveState: preserveState);
}

Changing it would be breaking. So I don't think that's desirable.

rjpower commented 1 year ago

Of course, it's easy to work around. It's more that the default yields confusing behavior. As for breaking, perhaps the default can change with the next major version bump?

I can send a PR with a warning in the documentation, if that's acceptable.

rrousselGit commented 1 year ago

I don't think that warrants a change. That's how StreamBuilder works already anyway.

You can check ConnectionState during the transition if you want

Feel free to make a doc PR, sure

rrousselGit commented 1 year ago

Closing since that's not a bug