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 onEffects not called when subscriptions are removed #8

Merged
merged 8 commits into from May 23, 2021

Conversation

jonaskello
Copy link
Contributor

@jonaskello jonaskello commented May 23, 2021

The onEffects() function is not called if Program.subscription() returns undefined. This causes problems becuse the previous call may have returned a subscription and that caused a call to onEffects() which then did setup some subscription. Then the program is no longer interested in the subscription so it returns undefined from Program.subscription(). Now the effect manager should terminate the underlying subscription but since onEffects() is not called it does not.

The problematic code is here. It is an attempt at optimizing the calls to onEffects() and only call the effect managers that actually has gathered effects. This works well for Cmd becuase in that case undefined means there is no command to execute. However for subscriptions undefined means the program wants no subscriptions which is significant if the program previously wanted subscriptions.

This line only calls effect managers that has gathered effects. This PR will change to all effect managers are always called and the ones that does not have any Cmd or Sub will recieve undefined on those parameters.

@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #8 (4099835) into master (2e7a0e9) will increase coverage by 5.22%.
The diff coverage is 63.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage   63.44%   68.67%   +5.22%     
==========================================
  Files           8       11       +3     
  Lines         145      166      +21     
  Branches       26       28       +2     
==========================================
+ Hits           92      114      +22     
+ Misses         52       51       -1     
  Partials        1        1              
Impacted Files Coverage Δ
src/__tests__/helpers/mock-effect-manager.ts 50.00% <50.00%> (ø)
src/program.ts 79.68% <50.00%> (+5.91%) ⬆️
src/__tests__/helpers/mock-program.ts 87.50% <87.50%> (ø)
src/__tests__/helpers/mock-render.ts 100.00% <100.00%> (ø)
src/effect-manager.ts 88.88% <0.00%> (+33.33%) ⬆️

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 2e7a0e9...4099835. Read the comment docs.

@jonaskello jonaskello merged commit f7922dd into master May 23, 2021
@jonaskello jonaskello deleted the fix-sub-undefined branch May 23, 2021 14:50
@jonaskello jonaskello mentioned this pull request May 23, 2021
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.

None yet

1 participant