Skip to content
This repository has been archived by the owner on Nov 26, 2018. It is now read-only.

fix(connectObs): unsubscribe from observables when unmounting #92

Merged
merged 2 commits into from
Dec 12, 2017

Conversation

matthieuprat
Copy link
Collaborator

No description provided.

Copy link

@julien-meichelbeck julien-meichelbeck left a comment

Choose a reason for hiding this comment

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

A monster. 👾

@codecov
Copy link

codecov bot commented Dec 11, 2017

Codecov Report

Merging #92 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   96.52%   96.57%   +0.04%     
==========================================
  Files          63       63              
  Lines         461      467       +6     
==========================================
+ Hits          445      451       +6     
  Misses         16       16
Impacted Files Coverage Δ
src/connectObs.js 92.85% <100%> (+1.19%) ⬆️

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 6d3650a...57dc917. Read the comment docs.

@matthieuprat
Copy link
Collaborator Author

matthieuprat commented Dec 11, 2017

I tested this changeset against the entire @doctolib code base and it's all green on CI.

@gregberge gregberge merged commit 2caf2a1 into master Dec 12, 2017
@gregberge gregberge deleted the connectObs-fix branch December 12, 2017 10:11
@oliviertassinari
Copy link
Contributor

oliviertassinari commented Dec 13, 2017

Ça leak 🌊. Est-ce que c'est ça que j'avais sur l'agenda mobile docteur ?

@julien-meichelbeck
Copy link

C'est en rajoutant une feature sur l'agenda mobile docteur que j'ai remarqué ça, donc très probablement :)

à chaque swipe vers le prochain jour, on gardait tous les observables sur les jours précédents :'(

@oliviertassinari
Copy link
Contributor

Well done!

@gregberge
Copy link
Owner

Incroyable que l'on ne l'ai pas vu avant 😱

@julien-meichelbeck
Copy link

ouais ça faisait une requête xhr de plus à chaque swipe ...

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

Successfully merging this pull request may close these issues.

4 participants