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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(stop): implement store.stop() fn #76

Merged
merged 3 commits into from
Dec 12, 2016

Conversation

bengourley
Copy link
Contributor

Hey 馃憢 So I went ahead and whipped up an implementation for the stop() fn which I found wanting/needing for hot reload (#75). It was pretty simple to add. Let me know what you think.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 98.651% when pulling 7282343 on bengourley:stop into fe99501 on yoshuawuyts:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 98.651% when pulling 2749b7e on bengourley:stop into fe99501 on yoshuawuyts:master.

@yoshuawuyts
Copy link
Owner

yoshuawuyts commented Nov 29, 2016

Ah yeah this is clever - now what I wonder is if this leaks memory (I think it does)

edit: so what I'm saying is: if it does maybe we should find a different way? - should we care about memory being leaked? I think we should

@bengourley
Copy link
Contributor Author

Yeah, I guess even as the previous app goes out of scope, all of the calls to send fns returned from createSend() still exist and therefore all of the guts of that barracks store can't be gc'd.

My view is that given the current situation, which was for the entire app to continue running, we had a much bigger memory leak, so this keeps that leak from impacting performance in a huge way like it was.

I'd like to find a way to fully kill the app with no leaks at all, but I also wanted to keep the scope of changes small in order to make it easier/faster to merge. I wonder if we could incorporate this as a first pass, and then open another issue to try fully plugging the leak? LMK 馃榿

@bengourley
Copy link
Contributor Author

Sorry to pester you @yoshuawuyts, but is there anything I can do to help move this along?

I have a tiny patch waiting for choo (below) which takes choo-resume from a proof of concept with terrible performance into something that's properly useful and usable! Obviously all of that stuff can't land until this does so I'm super keen to get some sort of stop() functionality available.

Thank you!

diff --git a/index.js b/index.js
index 7d27baf..8a758ba 100644
--- a/index.js
+++ b/index.js
@@ -31,6 +31,7 @@ function choo (opts) {
   start.router = router
   start.model = model
   start.start = start
+  start.stop = _store.stop
   start.use = use

   return start

@coveralls
Copy link

coveralls commented Dec 12, 2016

Coverage Status

Coverage increased (+0.05%) to 98.651% when pulling c97da90 on bengourley:stop into fe99501 on yoshuawuyts:master.

@yoshuawuyts yoshuawuyts merged commit c8bd94c into yoshuawuyts:master Dec 12, 2016
@yoshuawuyts
Copy link
Owner

v9.1.0 馃帀

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

3 participants