sdebaun / sparks-cyclejs

45 stars 4 forks source link

React component unmount #31

Open wclr opened 8 years ago

wclr commented 8 years ago

https://github.com/sdebaun/sparks-cyclejs/blob/release/src/helpers/dom/index.js#L11

I think just using hook is not enogth for correct component unmount. I mean that when element removed from DOM unmount lifecycle methods on React component will not be called automatically.

TylorS commented 8 years ago

@whitecolor Thank you for the issue. I think you are definitely correct here. I'm not very familiar with React myself. Do you believe something like:

export const reactComponent = (Klass, attrs, hookname = 'update') => 
  div({
    hook: {
      [hookname]: ({elm}) => ReactDOM.render(<Klass {...attrs} />, elm),
+     destroy: ({elm}) => ReactDOM.unmountComponentAtNode(elm)
    }
  })

Would be enough to ensure all of the React specfics are being handled?

wclr commented 8 years ago

I think yes, it should work this way. Snabdom seems to have very good api for that, if it works fine for any kind of elements without perfomance penalty it is great.

virtual-dom has very poor API for hooks it actually has only init of snabbdom's.

is cycle-snabdom ready for prod?

TylorS commented 8 years ago

I would definitely say yes, it shares every single test that @cycle/dom does (minus one regarding virtual-dom widgets), and then some more that are specific to snabbdom.

sdebaun commented 8 years ago

@whitecolor thank you for the suggestion! I'm wondering if it might be worth it to extract this into a simple snabbdom-react-component package?

wclr commented 8 years ago

I think no reason to extract it to package yet, it probably doesn't cover all the use cases.