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
StoreConnector.shouldUpdateModel and VmFactory.fromStore called with different state instances #89
Comments
I will give this priority, but could you please provide some reproducible code? Maybe even better, a failing test? |
Not sure, I will try. The code I have is not easy to reproduce. I think it is somehow related to the use of |
Looking at the source I think I found the potential problem site inside if (widget.shouldUpdateModel != null) {
_stream = _stream.where((state) => widget.shouldUpdateModel(state));
}
stream = _stream.map((_) => getLatestValue()); The state that is used to test |
We inherited this bug from flutter_redux. I opened an issue there: brianegan/flutter_redux#196 I did a detailed analysis of the architecture both projects use, and I'd say important changes must be made to solve this. Also the current architecture has performance problems, where the view-model is unnecessarily calculated more than it needs to (see brianegan/flutter_redux#197). I have already fixed the performance problems, but I'm still deciding how to fix the |
@kuhnroyal Could you please test it, and tell me if the problem is solved?
|
@marcglasberg This looks good, thank you! |
This works mostly but I still have a case where this happens. Here is some logging with comments.
I am not sure I can reproduce this in a simple test. Basically what I am doing is a logout procedure, clearing user state and removing all routes. The removed routes still get a rebuild with the wrong state. The await dispatchFuture(ClearUserDataAction());
await dispatchFuture(PersistAction());
await dispatchFuture(NavigateAction.pushNamedAndRemoveAll(AppRouter.login)); |
If you go to void _createStream() => _stream = widget.store.onChange
// This prevents unnecessary calculations of the view-model.
.where(_stateChanged)
// Discards invalid states.
.where(_shouldUpdateModel)
// Calculates the view-model using the `vm` or `converter` functions.
.map(_calculateModel)
// Don't use `Stream.distinct` because it cannot capture the initial
// ViewModel produced by the `converter`.
.where(_whereDistinct)
// Updates the latest-model with the calculated vm.
// Important: This must be done after all other optional
// transformations, such as shouldUpdateModel.
.transform(StreamTransformer.fromHandlers(
handleData: _handleData,
handleError: _handleError,
)); Relevant methods are Could you please check that code and see if you spot anything? Without a reproducible test this is quite hard to solve. |
I know, I will try to locate it. |
We have just migrated a project from |
I have tested this with @override
void didUpdateWidget(_StoreStreamListener<St, Model> oldWidget) {
_computeLatestModel(); // This throws and uses a new state which shouldUpdateModel returned false for
if (widget.store != oldWidget.store) {
_createStream();
}
super.didUpdateWidget(oldWidget);
} |
Yes, it would not have fixed itself, since I did not work in this for version 7. But it's on the radar. Ideally I should have a failing test with the |
Oh didn't mean to pressure you, I thought due to the new And I am also not sure that both my problems have the same source. Because the first one came from the stream and the current one is from a |
Don't worry, you are not pressuring me, you are helping. :) The |
@kuhnroyal I believe this is fixed now, so I am closing this. The tests are passing, and in my own apps I use this a few different times, and they work well. Please let me know if you are still having problems. Thank you! |
I am not on 12.0.0 in all projects but I will reopen if it pops up in the latest version. Thanks for your work on this project! |
Still getting this in my VmFactories, more often on slow devices. |
@marcglasberg I ran into this again today and tracked the problem to https://github.com/marcglasberg/async_redux/blame/master/lib/src/store_connector.dart#L408 Some external rebuild of the store connector causes a re-compute of the model from the Maybe this needs to be changed to |
lol thats the same thing I figured out one and half years ago, I just noticed from my comment above.... |
Managed to replicate this now very easily by adapting the existing Any external change, which may not be avoidable, can cause this to fail. |
It is possible for
StoreConnector.shouldUpdateModel
to be called with a different state instance thanVmFactory.fromStore
.So after verifying the correct state for the viewmodel, the state can change into an invalid state and
fromStore
is called with the invalid state.The text was updated successfully, but these errors were encountered: