create-subscription: call getValue in the constructor is too early

[lixiaoyan]

Do you want to request a feature or report a bug?

BUG

What is the current behavior?

https://codepen.io/intptr/pen/djEzbr?editors=1010

I made an example to show the execution order of some lifecycle functions while remounting a component:

newComponent.constructor() -> oldComponent.componentWillUnmount() -> newComponent.componentDidMount()

create-subscription calls getValue in constructor and save the result to its state. Before componentDidMount called, any changes will be ignored.

If I remount a component wrapped by create-subscription component, and do something in its componentWillMount which will modify the source value, I will get the wrong value in componentDidMount in the new component.

newComp.constructor -> oldComp.cWU -> newComp.cDM
state = A              A -> B         A !! (correct: B)

What is the expected behavior?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.4.2 (latest)

[gaearon]

create-subscription calls getValue in constructor and save the result to its state. Before componentDidMount called, any changes will be ignored.

That's right. However, it checks if the value has changed in componentDidMount. So that case should be handled.

If I remount a component wrapped by create-subscription component, and do something in its componentWillMount which will modify the source value, I will get the wrong value in componentDidMount in the new component.

Can you provide a live example demonstrating this? I don't think I fully understand. It's not clear to me how the example you provided corresponds to create-subscription. An example that actually uses create-subscription would be helpful.

[lixiaoyan]

const source = new BehaviorSubject({
  status: "initial" | "loading" | "error" | "aborted" | "complete",
})

const controller = {
  load: () => { ... },
  abort: () => { ... },
}

const Subscription = createSubscription(...)

class Content extends React.Component {
  componentDidMount() {
    if (["initial", "aborted"].includes(this.props.status)) {
      controller.load()
    }
  }

  componentWillUnmount() {
    controller.abort()
  }

  render() { ... }
}

class Container extends React.Component {
  render() {
    return (
      <Subscription source={source}>
        {({status}) => <Content status={status} />}
      </Subscription>
    )
  }
}

If I remount Container during loading:

newContent.constructor() (save {status: "loading"} to its state)
oldContent.componentWillUnmount() (status: "loading" -> "aborted")
newContent.componentDidMount() (status === "loading", will not try to reload)

[gaearon]

Is BehaviorSubject from Rx? Could you provide an example I could easily run (e.g. on CodeSandbox)? Sorry for the extra work — it just takes a lot of time to look at all issues so preparing runnable example greatly increases my chances of looking in depth.

[lixiaoyan]

@gaearon I tried to make a runnable example yesterday and I found there's no umd version of create-subscription on npm and then I gave it up 😂 I will take a look into CodeSandbox and try to make an online example later.

[lixiaoyan]

https://codesandbox.io/s/wqjq0xml5

@gaearon
You can click "remount" button during loading status (hard-coded time 10s) and see what happened.
Expected behavior: in componentDidMount I will get status = aborted and try to reload.
Current behavior: in componentDidMount I get status = loading (which is incorrect).

[gaearon]

This doesn't look like a bug to me.

Eventually render gets the right value. But you're not guaranteed to have up-to-date value in this.props inside componentDidMount.

What happens is that the child's componentDidMount runs first. By that time we don't know it's outdated yet. Then the parent's componentDidMount runs. This is when we know that the value has changed. Parent schedules a state update. However by this time child's componentDidMount has already run so it's too late to hope its props will reflect this update.

It seems to me that logic that only checks in componentDidMount is insufficient. If you add a similar check to componentDidUpdate (possibly also checking that prevProps didn't have this status — to avoid doing extra work on every update) it should be sufficient.

[lixiaoyan]

@gaearon
It could be aborted by a user (e.g. by clicking a button labelled "Abort"), in this case, it shouldn't try to reload anymore. But If I remount this component, it should try to reload, even if it has been aborted by a user before.

loading -> remount -> loading
loading -> user abort -> aborted
loading -> user abort -> aborted -> remount -> loading

So checking status in componentDidUpdate is not enough to detect whether it's aborted by a user or not.

[lixiaoyan]

Add another status, "user-aborted", may resolve this problem, but it makes the code more complex.
I should check status === "initial" || status === "aborted" || status === "user-aborted" in componentDidMount, and check status === "aborted" in componentDidUpdate, which is so confusing.

[lixiaoyan]

In my opinion, the main problem is, why this.props.status !== state.getValue().status in componentDidMount?
Should I use create-subscription to get the value for any case (check the value and do something in lifecycle), or only for render?

[gaearon]

My mental model for create-subscription is that it guarantees that the rendered value on the screen is the most recent one, and that subscription is cleaned up on unmount. You can think of it as a "pull" interface to the underlying value. In fact I'm not even sure we guarantee that componentDidUpdate is called for every intermediate value. Only the (eventually) latest one.

So in my view create-subscription isn't a great place for async control flow mechanics you're trying to implement. Think of it more like a "final consumer" for that async value. If you want to control it, and you already use Rx, it sounds like moving that outside of component into some sort of observables + subscriptions + combinators would make more sense? And then create-subscription lets you pull and display the latest values.

why this.props.status !== state.getValue().status in componentDidMount

Because props are the "latest pulled" copy. We'll pull again before the screen gets updated. But there's no guarantee that in any particular lifecycle you're reading the most recent value in props. Rather, the guarantee is that what will end up on the screen reflects the latest value. For the case where the value changes between the initial render and the mount, that second "pull" will happen when the parent wrapper mounts (and will cause a child update).

[lixiaoyan]

If you want to control it, and you already use Rx, it sounds like moving that outside of component into some sort of observables + subscriptions + combinators would make more sense?

There's no difference between these two things IMHO.
In both ways, we rely on lifecycle (mount and unmount), check status, and then do something (load or abort).
The only difference is how to get the status: from create-subscription or from the subject directly.

[lixiaoyan]

Anyway, thanks for your detailed explanation.

[gaearon]

The difference is that this.props is more like an "eventually consistent pull" mechanism. Unlike Rx combinators which are guaranteed to execute on every value change when it happens.