FormidableLabs / freactal

Clean and robust state management for React and React-like libs.
MIT License
1.65k stars 46 forks source link

Child component that always gets initial state, never updates #42

Closed jake-nz closed 7 years ago

jake-nz commented 7 years ago

In the following code, I have a parent component providing state with an effect to toggle a value. There are 4 copies of a child component that displays and toggles the value. The 4 are mounted and wrapped in different ways. All 4 can use the effect to toggle the value, only 3 ever show the updated value.

For the life of me, I can't figure out why that one doesn't work! Am I doing something weird? Is this a bug?

Thanks in advance!

import React from 'react'
import { provideState, injectState, softUpdate } from 'freactal'

const wrapParentWithState = provideState({
  initialState: () => ({
    toggleMe: true
  }),
  effects: {
    toggle: softUpdate(state => ({ toggleMe: !state.toggleMe }))
  }
})

const ParentWithState = wrapParentWithState(() => (
  <Parent>
    props.children: <Child />
    <hr />
    BROKEN props.children, with provideState: <ChildWithState />
  </Parent>
))

const Parent = injectState(({ state: { toggleMe }, children }) => (
  <div>
    <div>Parent: toggleMe is {toggleMe ? 'true' : 'false'}</div>
    <hr />
    <div>{children}</div>
    <hr />
    <div>direct: <Child /></div>
    <hr />
    <div>direct, with provideState: <ChildWithState /></div>
  </div>
))

const Child = injectState(({ state: { toggleMe }, effects: { toggle } }) => (
  <div>
    <button onClick={() => toggle()}>Toggle</button>
    toggleMe is {toggleMe ? 'true' : 'false'}
  </div>
))

const wrapChildWithState = provideState({
  initialState: () => ({}),
  effects: {}
})

const ChildWithState = wrapChildWithState(Child)

export default ParentWithState
divmain commented 7 years ago

Going to tag this as a potential bug, and dig into it over the next day or so. Thanks for the fantastic repro!

jake-nz commented 7 years ago

A Clue... receiveComponent in ReactReconciler.js does not run if the new element and context are equal to the old element and context. nextElement === prevElement is true for the broken element and removing this optimisation from React fixes my problem.

As to why nextElement === prevElement for "props.children, with provideState" but not for "direct, with provideState" I'll keep digging...

ReactReconciler.js:

  /**
   * Update a component using a new element.
   *
   * @param {ReactComponent} internalInstance
   * @param {ReactElement} nextElement
   * @param {ReactReconcileTransaction} transaction
   * @param {object} context
   * @internal
   */
  receiveComponent: function (internalInstance, nextElement, transaction, context) {
    var prevElement = internalInstance._currentElement;

    if (nextElement === prevElement && context === internalInstance._context) {
      // Since elements are immutable after the owner is rendered,
      // we can do a cheap identity compare here to determine if this is a
      // superfluous reconcile. It's possible for state to be mutable but such
      // change should trigger an update of the owner which would recreate
      // the element. We explicitly check for the existence of an owner since
      // it's possible for an element created outside a composite to be
      // deeply mutated and reused.

      // TODO: Bailing out early is just a perf optimization right?
      // TODO: Removing the return statement should affect correctness?
      return;  // <----- Here be dragons!
    }

    // ...
henryluki commented 7 years ago

I thought that this bug maybe was caused by provideState under directly wrapper by provideState. In this case,

 BROKEN props.children, with provideState: <ChildWithState />

This component should subscribe state change and update itself, maybe we should call forceUpdate in relayUpdate the method of provide.js.

jake-nz commented 7 years ago

Am I right in interpreting the heirachy as: <ParentWithState> (provideState) -> <Parent> (injectState) -> <ChildWithState> (provideState) -> <Child> (injectState)?

Also are "direct, with provideState" and "BROKEN props.children, with provideState" truly at the same place in the hierarchy?

henryluki commented 7 years ago

@jake-nz oh, I made a mistake. But that component props.children should update. You can try this code:

relayUpdate (changedKeys) {
    const relayedChangedKeys = this.invalidateChanged(changedKeys);
    this.forceUpdate() // add this line
    this.subscribers.forEach(cb => cb && cb(relayedChangedKeys));
 }

But, hope @divmain will have a good solution.

divmain commented 7 years ago

I believe I understand what's going on here, and it has to do with the ordering of update subscriptions and component rendering. I expect to have a fix up in the next couple of days - lots going on right now!

jake-nz commented 7 years ago

@henryluki, your suggestion works for me but I think performance has suffered due to excessive updates. It's a great option so I can continue development until it's fixed in freactal. Thanks, @divmain for your hard work!

divmain commented 7 years ago

Sorry for the wait everybody - I've been trying to get a feel for the general feedback and requested changes before jumping in. But a fix for this is now in master, and will be released shortly.

henryluki commented 7 years ago

I've see that changes, updating childContext by change its reference, brilliant!