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

async action behaves differently from 2.0.7+3. #834

Closed
amondnet opened this issue Jun 16, 2022 · 10 comments · Fixed by #842
Closed

async action behaves differently from 2.0.7+3. #834

amondnet opened this issue Jun 16, 2022 · 10 comments · Fixed by #842
Assignees

Comments

@amondnet
Copy link
Collaborator

amondnet commented Jun 16, 2022

  1. example
    @observable
    String stuff = '';
    
    @observable
    loading = false;
    
    @action
    Future<void> loadStuff() async {
      loading = true;
      stuff = await fetchStuff();
      loading = false;
    }
  2. test
    test('run allows updating observable values in an async function',
            () async {
          final action = AsyncAction('testAction');
    
          final counter = Observable<int>(0);
    
          final values = <int>[];
          autorun((_) {
            values.add(counter.value);
          });
    
          await action.run(() async {
            await sleep(10);
            counter
              ..value = 1
              ..value = 2;
            await sleep(10);
            counter.value = 3;
          });
    
          expect(counter.value, equals(3));
          expect(values, equals([0, 2, 3]));
       });

#89

mobx: <= 2.0.7+2

  1. example
    Future<void> loadStuff() async {
      runInAction(() {
        loading = true;
      })
      final _stuff = await fetchStuff();
      runInAction(() {
        stuff = _stuff;
        loading = false;
      });
    }
  2. test
    values : [0, 2, 3]

#784

mobx: >= 2.0.7+3

  1. example
    Future<void> loadStuff() async {
      runInAction(() {
        loading = true;
        final _stuff = await fetchStuff();
        stuff = _stuff;
        loading = false;
      });
    }
  2. test
    values : [0, 3]

https://github.com/mobxjs/mobx.dart/runs/6912223161?check_suite_focus=true

https://mobx.netlify.app/api/action/

Changes to the observables are only notified at the end of the action. This ensures all mutations happen as an atomic unit. This also eliminates noisy notifications, especially if you are changing a lot of observables (say, in a loop).

https://mobx.js.org/actions.html#updating-state-using-actions

They are run inside transactions. No reactions will be run until the outer-most action has finished, guaranteeing that intermediate or incomplete values produced during an action are not visible to the rest of the application until the action has completed.

https://mobx.js.org/actions.html#asynchronous-actions

According to the docs, the 2.0.7+3 seems to be working correctly.

edited:

According to the docs, the 2.0.7+2 seems to be working correctly.

@s0nerik
Copy link
Contributor

s0nerik commented Jun 16, 2022

This is an interesting topic to discuss.

Per-documentation vs non-informed user expectations

It seems to me that the behavior described in the docs matches non-informed user expectations only in the case of a synchronous method.

In the async case, the expectation of everyone I've asked was that the following

@action
Future<void> loadStuff() async {
  loading = true;
  stuff = await fetchStuff();
  loading = false;
}

would result in the loading == true being notified before the await happens, and then stuff == <something> and loading == false would be notified simultaneously after await'ed content is resolved.

But, according to the docs, these are wrong expectations.

Too broad interpretation of doc's notification grouping rule

It seems like the library implementation interprets the rule about notifications grouping even broader than it's written.

Nested actions won't trigger notifications until the outermost action body is fully executed

The way it works in the library - any observable changes within nested action invocations will also not_notify until the outermost action finishes its execution.

So, even if we do something like

@action
void setLoading(bool value) {
  loading = value;
}

@action
Future<void> loadStuff() async {
  setLoading(true);
  stuff = await fetchStuff();
  setLoading(false);
}

we still won't get the loading == true notification before await happens.

Even if the referenced action/observable_field belongs to a different state store - it won't get any value change notifications until the outermost action body is fully executed in a different state store

This too_broad interpretation of the rule gets even worse when we consider that there is no difference in having the nested action call belonging to the same state store and a separate one.

So, the behaviors of this

class StateStore1 {
  @observable
  Object? someResult;

  @observable
  bool busy = false;

  @action
  Future<void> doStuff() async {
    busy = true;
    someResult = await doSomething();
    busy = false;
  }
}

and this

class BusyStateStore {
  @observable
  bool busy = false;
}

class StateStore1 {
  final BusyStateStore busyStore;

  @observable
  Object? someResult;

  @action
  Future<void> doStuff() async {
    busyStore.busy = true;
    someResult = await doSomething();
    busyStore.busy = false;
  }
}

are the same in terms of observable change notification. None of them will notify the "busy == true" before await ends.

Specific common problematic case: navigation to a different page within action renders the page unable to receive any mobx notifications

This is especially problematic in the case of trying to do navigation to a page with a different state store within the async action body:

class StateStore {
  @observable
  Object? someResult;

  @action
  Future<void> doStuff(NavigatorState navigator) async {
    someResult = await navigator.pushNamed('/some_page');
    await Future.delayed(Duration(seconds: 2));
  }
}

If /some_page depends on anything mobx-related - you're in trouble. This page won't receive any observable change notifications until you pop it and 2 seconds pass, after which it will receive a whole bunch of notifications, which you, sadly, no longer care about. This might even break something on the already-popped page since its element has already been disposed of when all these notifications arrive, and handling them not carefully is quite easy.

How my team makes sure we don't get into the trouble with async action blocking any notifications?

We decided that disallowing the async actions altogether is the only way to ensure we don't run into the problems described above without spending a lot of time on the library itself.

We've introduced a, quite hacky, yet working, alternative in form of

Future<T> asyncAction<T>(Future<T> Function() function) => function();

which we use like this:

  @action
  Future<void> loadSomething() => asyncAction(() async {
        loading = true;
        try {
          result = await loadResult();
        } finally {
          loading = false;
        }
      });

This ensures that mobx_codegen will generate Action, not AsyncAction, for loadSomething, which matches non-informed user expectations much better and doesn't have the problems I've described above.

How to solve the problems described above in the library?

In my opinion, the library should perform notification batching for async actions according to the following rules:

  • batch notifications for synchronous parts (code in-between the awaits) of async actions as if they were simple, synchronous, actions themselves
  • batch notifications only within the same state store, letting other state stores notify value changes even if they're are referenced within another state store's action

@abodehBabelli
Copy link

version 2.0.7+3 also breaks our already working code, in our case we are using then with future, and then we are updating a value that should notify another computed value and it is not doing this as expected in version 2.0.7+3 but it is doing it correctly in version 2.0.7+2

@Shahed-Atik
Copy link

@abodehBabelli ++
same issue

@idrats
Copy link

idrats commented Jun 21, 2022

Also waiting for fix

@RizanPSTU
Copy link

Same issue

@quanghd96
Copy link

same issue

@fzyzcjy
Copy link
Collaborator

fzyzcjy commented Jun 22, 2022

Even if this is desired behavior, shall we make a user-configurable flag, such that at least can disable this and revert to old behavior?

@Syscomp-Software
Copy link

O mesmo problema

@amondnet
Copy link
Collaborator Author

@fzyzcjy I think we should revert #784 .

@gabrielscr
Copy link

same problem here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants