Ref as an observable

[heyimalex]

This came up today trying to pass a funcSubject as a ref callback function. Because we're not actually subscribed when the ref callback fires (ie before componentDidMount), the value is just kind of lost.

Using a BehaviorSubject instead fixed the issue, but I thought I'd mention it for anyone else who ran into it.

Also I was wondering why you subscribe in componentDidMount and not the constructor? It seems like it potentially adds some unnecessary work. I imagine it's a "universal" thing but I don't have any experience with that...

[acdlite]

Hmm, I feel like there was a reason but I can't think of it now. Perhaps we can subscribe in the constructor...

Also can you explain to me why BehaviorSubject worked but funcSubject did not? Is it something we can fix?

[acdlite]

Oh yeah, it's to prevent calls to setState() before the component has mounted.

[heyimalex]

It's because funcSubject is a plain Rx.Subject, which is a straight up hot observable; if onNext is called and there are no subscribers, no one will ever see it. Rx.BehaviorSubject is similar, but it basically caches the last value passed to onNext, and whenever someone new subscribes it starts the sequence with that last cached value.

I'm not sure how it could be fixed. Changing funcSubject to BehaviorSubject isn't perfect because people may want to dynamically subscribe/unsubscribe, and starting with the last value means that one of those events you were intentionally not listening to is now somehow firing... at some strange point in the future.

Maybe preventing calls to setState could be done on the subscribe side? Something like wrapping childProps$ in a BehaviorSubject in the constructor and then doing what you do now with that. I'll think about it some more today. Rx is so simple when it works and so finicky when it doesn't :)

[acdlite]

Ah I see! Couldn't we just provide two different versions of funcSubject(), one hot and one not?

[heyimalex]

Yeah, that would work. Generally those two should cover most things; Subject is an event, BehaviorSubject is a value changing over time.

I really do think it's better to subscribe once and batch changes between construct and componentDidMount though. I'm having a hard time explaining why...

[acdlite]

@heyimalex There is only one subscription. Or are you referring to this part? https://github.com/acdlite/react-rx-component/blob/master/src/createRxComponent.js#L5-L9

I wish we could avoid that but it's the only way to set the initial state in the constructor. Do you have a suggestion for how this could be better?

[heyimalex]

Yeah, I was referring to that. I think I'm afraid of hot/cold observable confusion and subscribe time side effects, but I really can't think of a good rational example right now so it's probably fine.

I have a branch that avoids it in a different way but I'm still working on it.

[acdlite]

Thanks for investigating! It doesn't look like your branch sets the initial state in the constructor, though. We need that for the initial render.

[heyimalex]

It depends I think? Rx observables will execute synchronously if they can, so if childProps$ is synchronous the subscription will fire before I change onNext to batch. Demo. If it's asynchronous then you probably want to batch it up anyways.

[acdlite]

Ah, but I don't think you're allowed to use setState() inside the constructor. I'll try to find confirmation, but I'm pretty confident that's true.

[heyimalex]

Haha well the really roundabout way is just to do:

let onNext = childProps => this.state = childProps;
// then
onNext = childProps => this.batched = childProps;
// then
if (this.batched) { this.setState(this.batched); }
onNext = ::this.setState;

[acdlite]

Actually that's a great point — we can just update the observer to check if the component is mounted and do either setState() or assign to state as needed!

[acdlite]

The extra subscription has been removed. I'll leave this open until we update funcSubject to allow the option to be "cold." Or maybe we need a better name for funcSubject? Can't think of any, though.

[heyimalex]

This actually should resolve the issue with refs; as we're subscribed when the ref callback fires, the value isn't lost and so there's no need to use a BehaviorSubject.

That being said, adding a funcBehaviorSubject is super easy and would be convenient for people who know that they need it. Using Rx terminology here is probably ok because if you know you need to use it you're going to be reading the Rx documentation anyways.

But I actually re-reviewed that code in #6 and I think I see a problem! The ref callback will fire between constructor and componentDidMount, so any values emitted from childProps$ resulting from a ref callback will be set directly onto state (via this.state = childProps). I'm pretty sure that means they won't get picked up until the next setState call.

I checked up on the setState-in-constructor thing and it looks like you're right. Do you know if you can use componentWillMount with the class syntax, or is it superseded by the constructor? Because an easy fix is to just move that this.componentHasMounted = false; into there (albeit with a more accurate name).

[acdlite]

@heyimalex I'm not sure if componentWillMount works, will have to check.

We'll need to find another solution for refs, by the way, because ref callbacks are being removed in 0.14: https://facebook.github.io/react/blog/#dom-node-refs

[heyimalex]

I didn't read that as they are removing ref callbacks, just changing what refs on React.DOM elements resolve to. The last time I looked they considered string refs "legacy" and callback refs the way forward.

[acdlite]

Ah I hope you're right. That would be nice.