Event binding in componentDidMount

[pwmckenna]

doing event binding in componentWillMount means that if this view is ever rendered server side, you'll have a leak because componentWillUnmount will never be called. Not a big deal, but this demo was my first taste of using react, so it kind of formed my initial opinion of where event binding should occur.

[coveralls]

Coverage remained the same at 98.28% when pulling 03eaff8 on pwmckenna:patch-1 into e3bbf46 on firebase:master.

[jwngr]

Thanks for the PR (and for trying out the plugin)!

So I'm not totally familiar with the differences between componentWillMount and componentDidMount, but from looking at the React doc on the lifecycle methods, it looks like the former will always be called on both the client and server while the latter will only be called on the client. With that understanding, it's not clear to me why componentDidMount would be more correct than componentWillMount here. If we go with componentDidMount, then this plugin would not work on the server, which doesn't seem like desired behavior.

It's quite possible that I'm missing something here, so please enlighten me if so.

[jwngr]

Also, I just realized you submitted a change to an example, not the actual plugin, but I guess my question still stands as to why componentDidMount is more correct here than componentWillMount.

[pwmckenna]

I confess that I hadn't even checked to see if the plugin was doing the same thing. If we come to agreement, I'd be happy to make a pull request for that as well.

the issue is that on the server the component is never mounted at all. this means both that componentDidMount is never called, but also that componentWillUnmount is never called. If the binding is done in componentWillMount, you'll leak if the component is never mounted (which is the case server side). Server side you wouldn't want to bind event handlers anyhow, as the rendering is done synchronously.

[pwmckenna]

just took a peek at the plugin src and realized that the plugin itself doesn't actually do event binding (its still early...brain not working...), so there's no leak, unless the consumer uses it incorrectly.

[jwngr]

Cool, thanks for the info. Going to close this PR then. Let me know if you have any other ideas!

[pwmckenna]

oh, i meant that the actual plugin isn't an issue. the example still would leak, it would just be the example's fault, not the plugin's.

[jwngr]

What exactly is being leaked here? The Firebase client will clean up after itself even if the user (or the plugin for that matter) doesn't remove the event listeners explicitly. So, we should be good there. Is there something else which is being leaked? Also, I still don't quite fully understand how the Firebase data would ever be downloaded on the server if we change this to componentDidMount, which is not going to be called. In that case, we would have no data, which seems bad.

[pwmckenna]

The plugin can't clean up after itself server side because componentWillUnmount is never called server side. I don't believe you'd want to use the plugin to get the data server side anyhow, because the rendering server side is synchronous, which means that you'll have to render before any of the callbacks are called anyhow. You'd want to wait for the data outside of the view and pass it in as an initialState type of prop so that the server could actually render synchronously.

ps. lots of discussion for just an example. I'm happy to go on, but cut me off whenever you get tired :)