search

marcglasberg / async_redux

Flutter Package: A Redux version tailored for Flutter, which is easy to learn, to use, to test, and has no boilerplate. Allows for both sync and async reducers.
Other
234 stars 40 forks source link

Account for existing _forceLastValidStreamState when rebuilding #142

Closed kuhnroyal closed 2 years ago

kuhnroyal commented 2 years ago
kuhnroyal commented 2 years ago

There are 2 test failures in the persistence test but that has to be unrelated.

marcglasberg commented 2 years ago

Yes, the persistence code is correct, but the persistence tests that are failing because they depend on precise timing. I was hoping to think of a way to make them more stable, but maybe they need to be removed.

marcglasberg commented 2 years ago

Ideally we should first create a failing test that was fixed by this change.

kuhnroyal commented 2 years ago

Ideally we should first create a failing test that was fixed by this change.

I agree but that needs to be a widget test. Will see what I can do.

marcglasberg commented 2 years ago

Yes, if the problem is in the didUpdateWidget you need a widget test. Thank you!!

kuhnroyal commented 2 years ago

I added a test case which fails without the change.

I also needed to make one more change in the StoreConnector. Without setting the initial state as _mostRecentValidState, it could still fail if shouldUpdateModel returns false for the first state change. In that case _mostRecentValidState would still be null in line https://github.com/marcglasberg/async_redux/blob/master/lib/src/store_connector.dart#L477

kuhnroyal commented 2 years ago

I added a test workflow, if you want to enable Github Actions.

kuhnroyal commented 2 years ago

Additionally I found with my test that the converter does not work together with shouldUpdateModel. The incorrect state is passed to the converter function in https://github.com/marcglasberg/async_redux/blob/master/lib/src/store_connector.dart#L600

kuhnroyal commented 2 years ago

We could an an assert or change the converter function parameter, both changes are breaking.

marcglasberg commented 2 years ago

What changes to the converter function do you recommend?

kuhnroyal commented 2 years ago

Fixed the typo.

What changes to the converter function do you recommend?

I would say, release the fix as a patch and change the parameter of the converter function in a breaking release. So far nobody seems to have this problem, I hardly ever use the converter and only noticed this with the test case.

marcglasberg commented 2 years ago

@kuhnroyal Your fix is published in version 17.0.1. Thank you!