yoshuawuyts / barracks

:mountain_railway: action dispatcher for unidirectional data flows
MIT License
177 stars 22 forks source link

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

Closed bengourley closed 7 years ago

bengourley commented 8 years ago

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 commented 8 years ago

Coverage Status

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

coveralls commented 8 years ago

Coverage Status

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

yoshuawuyts commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Coverage Status

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

yoshuawuyts commented 7 years ago

v9.1.0 :tada: