mobx.dart icon indicating copy to clipboard operation
mobx.dart copied to clipboard

Fix: AsyncAction behavior

Open lamnhan066 opened this issue 3 years ago • 3 comments

Hello,

I found this issue when I test my last PR. After doing some searching, I found that the PR #784 shouldn't be introduced without a breaking change version.

So I decided to fix this issue without breaking the code of the old users (who may be used this package since 2019 with PR #89) and allows anyone who wants to use new behavior (which is the official behavior of Mobx in the document).

I add an asyncActionBehavior flag to ReactiveConfig to control the behavior of @action on Future methods. This config is just needed for those who want to use new behavior to avoid breaking old users' code.

  • New behavior:
/// The same behavior with mobx >= 2.0.7+3
abstract class _YourStore with Store {
  @override
  ReactiveContext get context {
    return mainContext
      ..config.clone(asyncActionBehavior: AsyncActionBehavior.notifyOnlyLast);
  }
}
  • Old behavior (currently users don't need to add this code):
/// The same behavior with mobx <= 2.0.7+2
abstract class _YourStore with Store {
  @override
  ReactiveContext get context {
    return mainContext
      ..config.clone(asyncActionBehavior: AsyncActionBehavior.notifyEachNested);
  }
}

Fixes: #834 Fixes: #836 Fixes: #840


Pull Request Checklist

  • [x] If the changes are being made to code, ensure the version in pubspec.yaml is updated.
  • [x] Increment the major/minor/patch/patch-count, depending on the complexity of change
  • [x] Add the necessary unit tests to ensure the coverage does not drop
  • [x] Update the Changelog to include all changes made in this PR
  • [x] Run the set:versions command using npm or yarn. You can find this command in the package.json file in the root directory
  • [ ] Include the necessary reviewers for the PR
  • [ ] Update the docs if there are any API changes or additions to functionality

lamnhan066 avatar Jul 05 '22 16:07 lamnhan066

Deploy Preview for mobx canceled.

Name Link
Latest commit b8573c4da7d6d0bfe81e07a3635194a38621c159
Latest deploy log https://app.netlify.com/sites/mobx/deploys/62c46a995e09100008915a7d

netlify[bot] avatar Jul 05 '22 16:07 netlify[bot]

@vursin Thank you for your contribution. I think #784 should be reverted(#842). After that, we need to discuss the async action.

amondnet avatar Jul 06 '22 01:07 amondnet

@amondnet Additional information: This PR has the same behavior as reverted #784, just add an additional flag for those who want to continue using the new behavior.

lamnhan066 avatar Jul 06 '22 03:07 lamnhan066

Hi all,

Does this just need a final review before we can merge and close it?

Thanks, Gavin.

ghenry avatar Dec 19 '22 11:12 ghenry

@vursin Hello and thanks for the PR!

However, I'm not sure if we need this feature. Are there cases where this feature is needed? How often is that case? whole app, some store or some method? Does this pr solve that case?

How about something like below?

@action(batch=true) //
Future<void> runInBatch() {
}

amondnet avatar Jan 30 '23 17:01 amondnet

@amondnet I think this feature is also not really needed myself but it will be better if there is a way to just notify when all the actions inside are finished.

I also think this is a better way to do that so feel free to close this PR.

How about something like below?

@action(batch=true) //
Future<void> runInBatch() {
}

lamnhan066 avatar Jan 31 '23 06:01 lamnhan066