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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React component unmount #31

Open
wclr opened this issue Mar 5, 2016 · 5 comments
Open

React component unmount #31

wclr opened this issue Mar 5, 2016 · 5 comments
Labels

Comments

@wclr
Copy link

wclr commented Mar 5, 2016

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 TylorS added the bug label Mar 6, 2016
@TylorS
Copy link
Collaborator

TylorS commented Mar 6, 2016

@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
Copy link
Author

wclr commented Mar 6, 2016

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
Copy link
Collaborator

TylorS commented Mar 6, 2016

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
Copy link
Owner

sdebaun commented Mar 6, 2016

@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
Copy link
Author

wclr commented Mar 7, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants
@sdebaun @wclr @TylorS and others