Widdershin / cycle-restart

Swap out the code in your Cycle.js apps on the fly!
MIT License
123 stars 11 forks source link

Proper cleanup while DOM driver disposal #43

Closed wclr closed 7 years ago

wclr commented 8 years ago

This isssue is in cycle repo: https://github.com/cyclejs/core/issues/263 The problem is that when just dispose DOM driver sink/sources there maybe some leaks (destory hooks do not fire) after that because vdom is not cleaned up properly.

Widdershin commented 8 years ago

@whitecolor is there anything you think can be achieved in the cycle-restart project to address this issue?

If you have an example of an app that exhibits this problem, I would love to see it.

wclr commented 8 years ago

You may try code below on your http://widdersh.in/tricycle/ As div added to DOM hook method called. As div removed from DOM unhook method called.

You can see on restart that uhook method is not called But if you replace standard makeDOMDriver call with modified _makeDOMDriver DOM: restartable(_makeDOMDriver('.app'), {pauseSinksWhileReplaying: false}) you will see that unhook is called on restart now

Why it is important to call all the destory hooks? There maybe some some timeouts or othere bindings exist when whe use hooks to create widgets. For example in this issue https://github.com/sdebaun/sparks-cyclejs/issues/31 discussion that attched via hooks React component should be unmounted property on destroy.

const Cycle = require('@cycle/core');
const {makeDOMDriver, div, button} = require('@cycle/dom');
const _ = require('lodash');
const {Observable} = require('rx');
const {BehaviorSubject} = require('rx');
const {restartable} = require('cycle-restart');

var Hook = function(){}

Hook.prototype.hook = function(el){
  var i = 0
  console.log('I am hooked')
}

Hook.prototype.unhook = function(){
  console.log('I am unhooked')
}

const _makeDOMDriver = (contaner, options) =>
  (dom$) => {
    let empty$ = new BehaviorSubject()
    empty$.onNext(false)
    let source = makeDOMDriver(contaner, options)(
      Observable.combineLatest(dom$, empty$, (dom, empty) => {
        return empty ? div('') /* empty dom */ : dom}
      )
    )
    let _dispose = source.dispose
    source.dispose = () => {
      empty$.onNext(true)
      _dispose.apply(source)
    }
    return source
  }

function main ({DOM}) {
  const add$ = DOM
    .select('.add')
    .events('click')
    .map(ev => 1);

  const count$ = add$
    .startWith(0)
    .scan((total, change) => total + change)

  return {
    DOM: count$.map(count =>
      div('.counter', {'hook': new Hook()}, [
        'Count: ' + count,
        button('.add', 'Add')
      ])
    )
  };
}

const sources = {
  DOM: restartable(makeDOMDriver('.app'), {pauseSinksWhileReplaying: false})
}
Widdershin commented 7 years ago

@whitecolor can you please check if this works correctly on the latest release now that the underlying issue in Cycle is closed?

wclr commented 7 years ago

I believe this is works.