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 reports to spy hundreds of times on each run #734

Closed
makstheimba opened this issue Jan 18, 2022 · 6 comments · Fixed by #784
Closed

Async action reports to spy hundreds of times on each run #734

makstheimba opened this issue Jan 18, 2022 · 6 comments · Fixed by #784
Labels
bug Something isn't working

Comments

@makstheimba
Copy link

Hi! We've added a spy for mobx so we can see in console what is happening with our stores. However it's not really usable for us because for some reasons async actions report to spy hundreds of times during a coarse of single action. I was able to reproduce this issue using HackerNews example from mobx tutorial.

To reproduce:

  1. Replace HackerNewsStore in news_store.dart with the one that has async action instead of FutureObserver:
abstract class _HackerNewsStore with Store {
  final _hnApi = HNApi();

  @observable
  List<FeedItem> latestItems = [];

  @action
  Future<void> loadNews(FeedType type) async {
    if (type == FeedType.latest) {
      latestItems = await _hnApi.newest();
    } else if (type == FeedType.top) {
      topItems = await _hnApi.top();
    }
  }
  1. Replace FeedItemsView from news_widgets.dart to use only latestItems from store for simplicity of example:
class FeedItemsView extends StatelessWidget {
  const FeedItemsView(this.store, this.type, {Key? key}) : super(key: key);

  final HackerNewsStore store;
  final FeedType type;

  @override
  // ignore: missing_return
  Widget build(BuildContext context) => Observer(builder: (_) {
        final List<FeedItem> items = store.latestItems;
        return ListView.builder(
            physics: const AlwaysScrollableScrollPhysics(),
            itemCount: items.length,
            itemBuilder: (_, index) {
              final item = items[index];
              return ListTile(
                leading: Text(
                  '${item.score}',
                  style: const TextStyle(fontSize: 20),
                ),
                title: Text(
                  item.title,
                  style: const TextStyle(fontWeight: FontWeight.bold),
                ),
                subtitle: Text('- ${item.author}'),
                onTap: () => store.openUrl(item.url),
              );
            });
      });
}
  1. Enable Spying in main.dart:
mainContext.config = mainContext.config.clone(
    isSpyEnabled: true,
  );
  1. Run codegen - flutter packages pub run build_runner watch --delete-conflicting-outputs
  2. Run the project

Expectation:
loadNews action in HackerNewsStore runs only once and reports to spy about running once on Start and once on End.

Reality:
loadNews action in HackerNewsStore indeed runs only once (I can even test it by adding print statement inside async action) but reports to spy 450 (!) times about it's Start and End
image

Using:
flutter_mobx: 2.0.3
mobx: 2.0.6

@pavanpodila
Copy link
Member

Ya this definitely needs to be fixed. I am currently running short on bandwidth. I am happy to take a PR if you can and review and help you over there. Let me know if we can collaborate to fix this over a PR.

@makstheimba
Copy link
Author

makstheimba commented Jan 19, 2022

Thanks for quick response. I'll see if I have enough motivation to dig into this on the weekends :)

@roitalpaz
Copy link

is this in any roadmap to be fixed?
this renders the spy a bit useless for us :(

@amondnet amondnet added the bug Something isn't working label Mar 30, 2022
@pavanpodila
Copy link
Member

Let's get a PR rolling. I can review and we can put in the fix.

s0nerik added a commit to s0nerik/mobx.dart that referenced this issue Apr 2, 2022
s0nerik added a commit to s0nerik/mobx.dart that referenced this issue Apr 2, 2022
@s0nerik
Copy link
Contributor

s0nerik commented Apr 2, 2022

Here's the fix: #784

The issue was (the way I understand it) that the action zone's run and runUnary were overridden in a way that was treating each "continuation" (the synchronous part between two awaits) as a separate action, which is wrong - all "continuations" are just parts of the same async action and can happen really deep inside some library (in my case it was a single gRPC call that triggered lots of "buffer processing continuations" giving me a huge amount of unnecessary action events).

Nonetheless, I think it might be useful to have a (disabled by default) option to spy on these "continuations" as well. But in order to do that, a new kind of event has to be introduced, which must be traceable to the original action that triggered it. Since it looks like there's no easy way to achieve that right now, I'll just leave it as an idea for the future.

@danteCarvalho
Copy link

is this related to #791 ?

pavanpodila added a commit that referenced this issue Jun 11, 2022
* Fix for #734

* Update async_action.dart

* spy_test.dart: ensure that spy-event is raised only once for async actions that involve scheduleMicrotask

* Updated changelog

* Updated mobx version

* Removed unneeded newline

* Removed unneeded newline

Co-authored-by: Pavan Podila <[email protected]>
tlvenn pushed a commit to magelo-labs/mobx.dart_old that referenced this issue Dec 31, 2022
* Fix for mobxjs#734

* Update async_action.dart

* spy_test.dart: ensure that spy-event is raised only once for async actions that involve scheduleMicrotask

* Updated changelog

* Updated mobx version

* Removed unneeded newline

* Removed unneeded newline

Co-authored-by: Pavan Podila <[email protected]>
tlvenn pushed a commit to magelo-labs/mobx.dart_old that referenced this issue Dec 31, 2022
* Fix for mobxjs#734

* Update async_action.dart

* spy_test.dart: ensure that spy-event is raised only once for async actions that involve scheduleMicrotask

* Updated changelog

* Updated mobx version

* Removed unneeded newline

* Removed unneeded newline

Co-authored-by: Pavan Podila <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants