Skip to content
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

Fix: AsyncAction behavior #841

Closed

Conversation

lamnhan066
Copy link

@lamnhan066 lamnhan066 commented Jul 5, 2022

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

  • If the changes are being made to code, ensure the version in pubspec.yaml is updated.
  • Increment the major/minor/patch/patch-count, depending on the complexity of change
  • Add the necessary unit tests to ensure the coverage does not drop
  • Update the Changelog to include all changes made in this PR
  • 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

@netlify
Copy link

netlify bot commented Jul 5, 2022

Deploy Preview for mobx canceled.

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

@amondnet
Copy link
Collaborator

amondnet commented Jul 6, 2022

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

@lamnhan066
Copy link
Author

@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.

@ghenry
Copy link

ghenry commented Dec 19, 2022

Hi all,

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

Thanks,
Gavin.

@amondnet
Copy link
Collaborator

amondnet commented Jan 30, 2023

@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() {
}

@lamnhan066
Copy link
Author

@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() {
}

@amondnet amondnet closed this Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants