ReSub ComponentBase "swallowing" context updates

[lucasavila00]

It seems that ReSub swallows the context update on some specific use-case.
I'm having trouble using ReSub and React-Router, but I was able to reproduce it with just React.

The repro is the following:

import * as React from "react";
import { ComponentBase } from "resub";
import * as PropTypes from "prop-types";

class NotWorking extends ComponentBase<{}, {}> {
  render() {
    return <div>{this.props.children}</div>;
  }
}

class Working extends React.Component {
  render() {
    return <div>{this.props.children}</div>;
  }
}

class RenderFromContext extends React.Component {
  static contextTypes = { text: PropTypes.string };
  render() {
    return this.context.text;
  }
}

class MakeContext extends React.Component<
  {},
  { text: string }
> {
  state = {
    text: "",
  };
  static childContextTypes = { text: PropTypes.string };

  getChildContext() {
    return { text: this.state.text };
  }

  componentDidMount() {
    this.setState({ text: "Works" });
  }

  render() {
    return <div>{this.props.children}</div>;
  }
}

class App extends React.Component {
  render() {
    return (
      <MakeContext>
        Working:
        <Working>
          <RenderFromContext />
        </Working>
        Not working:
        <NotWorking>
          <RenderFromContext />
        </NotWorking>
      </MakeContext>
    );
  }
}

export default App;

If you'd like to run it on your browser I've made a create-react-app with this code and it is hosted here: https://github.com/degroote22/resubctxbug2

Just clone then npm install then npm start and you should see the following on your browser:
Working: Works Not working:

Thanks for your time 👍

[lucasavila00]

The problem lies on the current implementation of shouldComponentUpdate
Removing it completely resolves my issue but I wasn't able to run the tests on my machine too see if any would fail.

[deregtd]

Ah, so we need to take context into account there. Should be an easy fix.

[lucasavila00]

Please correct me if I'm wrong:
Since ReSub is doing a deep EqualTo and React also does it, the check on shouldComponentUpdate is worthless, right?
Also, there is a discussion on the React repository about this issue and it doesn't seem quite simple.
Anyway, thanks for your time 👍

[deregtd]

My understanding: React doesn't do a deep equalto for shouldcomponentupdate. If you don't provide a shouldcomponentupdate, it defaults to "yes".

[deregtd]

Published 1.0.3 that should address this. Let us know if it doesn't!

[ms-markda]

Unfortunately, your specific example shows a weakness in how context works in React. The only fix is to change the shouldComponentUpdate of the middle components, for example to return true like the default in React.Component. Or force a re-render of the entire (sub)tree through less subtle means.

class NotWorking extends ComponentBase {
    shouldComponentUpdate(nextProps, nextState, nextContext) {
        return true;
    }
}

For a counter example, change Working as follows and it will break. The this.context and nextContext are both empty (thus equal) since the component does not declare that it wants context.

class Working extends React.Component {
    shouldComponentUpdate(nextProps, nextState, nextContext) {
        return !_.isEqual(this.props, nextProps) || !_.isEqual(this.state, nextState) || !_.isEqual(this.context, nextContext);
    }
}

[ms-markda]

However, if you have no 'middle' components (or they all will update) then there was still a bug. The component that uses context was not re-rendering. PR #40 addressed that. Thanks for pointing that out.

[btraut]

Be very careful both using and updating context in your application. Best practices for context is described here. An excerpt:

1. Context should not change; it should be (shallow) immutable
2. Components should receive context only once; when they are constructed.

[lucasavila00]

It did not solve the issue. It's because of what @ms-markda said here:

The this.context and nextContext are both empty (thus equal) since the component does not declare that it wants context.

So, in order to make it work I'd have to put one of these 'methods/properties' onto NotWorking:

  static contextTypes = { text: PropTypes.string };

or

  shouldComponentUpdate() {
    return true;
  }

You'll always get an empty object for context unless you declare it's shape.
It's OK to declare it if I'm in charge of the context's shape, but in most cases I'm not.
The only true workaround I'm seeing is to use shouldComponentUpdate(){return true}.

My 2 cents:
Wait for React to release their new context API and document this issue while it cannot be truly fixed without affecting performance of ReSub.

PS: @btraut This issue came to surface while using React-Router. But, for instance, Styled-Components also uses it to broadcast changes on theme. I never use context explicitly on my apps, but lots of libraries do.