correctly fix deregister callbacks for OnUpdate (#188)

* revert change

* correctly fix deregister callbacks for OnUpdate

* correctly clean up observables + test

src/rendering/hyperscript_integration.jl

@@ -85,7 +85,7 @@ attribute_render(session::Session, parent, attribute::String, x::Nothing) = x
 attribute_render(session::Session, parent, attribute::String, x::Bool) = x
 
 function attribute_render(session::Session, parent, attribute::String, obs::Observable)
-    rendered = map(obs) do value
+    rendered = map(session, obs) do value
         attribute_render(session, parent, attribute, value)
     end
     onjs(session, rendered, js"value=> JSServe.update_node_attribute($(parent), $attribute, value)")

src/serialization/caching.jl

@@ -26,8 +26,9 @@ function register_observable!(session::Session, obs::Observable)
     # Only register one time
     if !haskey(root.session_objects, obs.id)
         updater = JSUpdateObservable(root, obs.id)
-        deregister = on(updater, obs)
-        push!(session.deregister_callbacks, deregister)
+        # Don't deregister on root / or session close
+        # The updaters callbacks are freed manually in delete_cached!`
+        on(updater, obs)
     end
     return
 end
@@ -93,7 +94,8 @@ function add_cached!(create_cached_object::Function, session::Session, send_to_j
     haskey(session.session_objects, key) && return result
     # Now, we have two code paths, depending on whether we have a child session or a root session
     root = root_session(session)
-    if root === session # we are root, so we simply cache the object (we already checked it's not cached yet)
+    # we are root, so we simply cache the object (we already checked it's not cached yet)
+    if root === session
         send_to_js[key] = create_cached_object()
         session.session_objects[key] = object
         return result
@@ -120,6 +122,12 @@ function child_has_reference(child::Session, key)
     return any(((id, s),)-> child_has_reference(s, key), child.children)
 end
 
+function remove_js_updates!(session::Session, observable::Observable)
+    filter!(observable.listeners) do (prio, f)
+        !(f isa JSUpdateObservable && f.session === session)
+    end
+end
+
 function delete_cached!(root::Session, key::String)
     if !haskey(root.session_objects, key)
         # This should uncover any fault in our caching logic!
@@ -135,9 +143,7 @@ function delete_cached!(root::Session, key::String)
         object = pop!(root.session_objects, key)
         if object isa Observable
             # unregister all listeners updating the session
-            filter!(object.listeners) do (prio, f)
-                !(f isa JSUpdateObservable && f.session === root)
-            end
+            remove_js_updates!(root, object)
         end
     end
 end

src/session.jl

@@ -89,6 +89,10 @@ function free(session::Session)
             end
         end
     end
+    # We need to remove all JSUpdateObservable from session observables
+    for (k, v) in session.session_objects
+        v isa Observable && remove_js_updates!(session, v)
+    end
     # delete_cached! only deletes in the root session so we need to still empty the session_objects:
     empty!(session.session_objects)
     empty!(session.on_document_load)

test/basics.jl

@@ -14,10 +14,15 @@
     session_cache = decoded[1]
     @test session_cache isa JSServe.SessionCache
     @test haskey(session_cache.objects, mapped.id)
+    @test haskey(session.session_objects, mapped.id)
 
     # Will deregisters correctly on session close
-    @test session.deregister_callbacks[1].observable === mapped
+    @test length(session.deregister_callbacks) == 1
+    @test session.deregister_callbacks[1].observable === obs1
     @test occursin("JSServe.update_node_attribute", string(decoded[2]["payload"]))
+    close(session)
+    @test isempty(obs1.listeners)
+    @test isempty(mapped.listeners)
 end
 
 struct DebugConnection <: JSServe.FrontendConnection