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

Don't trigger unneeded ActionSpyEvent for AsyncAction #784

Merged
merged 11 commits into from
Jun 11, 2022

Conversation

s0nerik
Copy link
Contributor

@s0nerik s0nerik commented Apr 2, 2022

Fixes #734

@netlify
Copy link

netlify bot commented Apr 2, 2022

Deploy Preview for mobx canceled.

Name Link
🔨 Latest commit 4f42d38
🔍 Latest deploy log https://app.netlify.com/sites/mobx/deploys/62a20f9dc01b310008fa5e5b

@codecov
Copy link

codecov bot commented Apr 2, 2022

Codecov Report

Merging #784 (4f42d38) into master (45248a9) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   98.81%   98.80%   -0.01%     
==========================================
  Files          56       56              
  Lines        1933     1931       -2     
==========================================
- Hits         1910     1908       -2     
  Misses         23       23              
Flag Coverage Δ
flutter_mobx 100.00% <ø> (ø)
mobx 98.32% <100.00%> (-0.01%) ⬇️
mobx_codegen 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mobx/lib/src/api/async/async_action.dart 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45248a9...4f42d38. Read the comment docs.

@s0nerik s0nerik changed the title Fix for #734 Don't trigger unneeded ActionSpyEvent for AsyncAction Apr 2, 2022
@fzyzcjy
Copy link
Collaborator

fzyzcjy commented Apr 3, 2022

Could you please add a bit of tests to verify it :)

@s0nerik
Copy link
Contributor Author

s0nerik commented Apr 3, 2022

Could you please add a bit of tests to verify it :)

Yes! I've added a test that verifies the correct behavior. It breaks on master (eventCount == 4) and works correctly with my patch (eventCount == 1).

I've also identified that my previous guess about the reasons for extra events being reported might've been wrong. I couldn't reproduce the problem using "continuations" in the test environment, but easily did it with scheduleMicrotask.

Copy link
Collaborator

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@pavanpodila pavanpodila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the Changelog, bump up the version with +2 on mobx. Make sure to invoke set-versions via npm or directly on terminal

@s0nerik s0nerik requested a review from pavanpodila May 9, 2022 15:27
@s0nerik
Copy link
Contributor Author

s0nerik commented May 26, 2022

@pavanpodila please take a look. I've updated the version as you requested.

@makstheimba
Copy link

Any progress on this? We are really waiting for this change as await is unusable without it

@pavanpodila
Copy link
Member

It's missing some changes to move forward

@s0nerik
Copy link
Contributor Author

s0nerik commented Jun 9, 2022

@pavanpodila I've already updated the versions as you requested, twice. But since other commits get merged (often overriding the version you've requested me to adjust) and this one stays without attention, I'm not sure when I need to apply the changes to make the merge happen.

I simply don't have enough time to monitor every commit in this repository to apply the version adjustment time and time again.

@pavanpodila
Copy link
Member

Let me check. I'm in transit now

@pavanpodila pavanpodila merged commit 4f98362 into mobxjs:master Jun 11, 2022
@pavanpodila
Copy link
Member

@all-contributors add @s0nerik for code

@allcontributors
Copy link
Contributor

@pavanpodila

@s0nerik already contributed before to code

@amondnet
Copy link
Collaborator

@pavanpodila This pr failed tests.

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

dart run test test/async_action_test.dart -N "run allows updating observable values in an async function"

00:00 +0 -1: AsyncAction run allows updating observable values in an async function [E]                                                                                                                     
  Expected: [0, 2, 3]
    Actual: [0, 3]
     Which: at location [1] is <3> instead of <2>
  
  package:test_api                  expect
  test/async_action_test.dart 42:7  main.<fn>.<fn>
  
00:00 +0 -1: Some tests failed. 

@lamnhan066 lamnhan066 mentioned this pull request Jul 5, 2022
7 tasks
amondnet added a commit to amondnet/mobx.dart that referenced this pull request Jul 6, 2022
@amondnet amondnet mentioned this pull request Jul 6, 2022
7 tasks
amondnet added a commit that referenced this pull request Jul 22, 2022
tlvenn pushed a commit to magelo-labs/mobx.dart_old that referenced this pull request 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 pull request Dec 31, 2022
tlvenn pushed a commit to magelo-labs/mobx.dart_old that referenced this pull request 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 pull request Dec 31, 2022
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 this pull request may close these issues.

Async action reports to spy hundreds of times on each run
5 participants