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(core/subscribe): stop rendering children when source$ switches #147

Merged
merged 7 commits into from
Nov 11, 2020

Conversation

voliva
Copy link
Collaborator

@voliva voliva commented Nov 9, 2020

I had an issue caused because I was receiving the old value from a factory bind(id => ...) when switching its key based on a router's param.

We realised this shouldn't happen when using Subscribe boundaries properly, but Subscribe doesn't currently handle the case of the source stream switching.

In here I add two tests: The first one tries to represent the underlying issue in Subscribe, but it turned out really contrived. So I also added a second test which better represents the issue I was having initially.

@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #147 (8b96080) into main (90e6a00) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #147   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          342       349    +7     
  Branches        41        43    +2     
=========================================
+ Hits           342       349    +7     
Impacted Files Coverage Δ
packages/core/src/Subscribe.tsx 100.00% <100.00%> (ø)
packages/core/src/bind/connectFactoryObservable.ts 100.00% <100.00%> (ø)
packages/core/src/bind/connectObservable.ts 100.00% <100.00%> (ø)
packages/core/src/internal/useObservable.ts 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 90e6a00...8b96080. Read the comment docs.

@josepot
Copy link
Member

josepot commented Nov 11, 2020

Thanks a lot for this @voliva ! I've added a bunch of commits with tiny improvements. Please, could you please re-review it before we merge?

@josepot josepot merged commit ae2c645 into main Nov 11, 2020
@josepot josepot deleted the subscribe-key-switching branch November 11, 2020 12:02
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

2 participants