react-side-effect: React v16: Error during mount causes instance to never be removed from mountedInstances

I also wonder whether the side effect should occur in componentDidUpdate or componentWillUpdate - there’s often a case for a side effect to occur before render rather than after

Side effects are only safe in the commit phase (after render).

I think maybe the right fix is for every component in their commit-phase hooks to find the root component (through context) and cause it to call setState on itself (maybe passing depth as an argument).

Components themselves just return this.props.children from render so that should always be memoized and setState on the root shouldn’t cause a deep re-render. Since we’re already inside the lifecycle, it should get batched and be processed after all such setStates. Resulting in a single “flush” after all inner components have updated their props.

Finally, the root component flushes changes in its own componentDidUpdate hook.

Issues like this is why we’re moving towards deprecating componentWillMount in the future.

I think it’s a bit shady that we currently track instances. This seems more like a use case for context. This also exposes a flaw in the model itself: it doesn’t consider that there can be siblings. So the stack abstraction is wrong in the first place:

<A>
  <B>
    <C> // should this win?
  </B>
  <D /> // or this?
</A>

I guess this API is still useful though. Although if it went through a major change that wouldn’t be a big deal because its use is pretty isolated. So maybe we can come up with something better right away instead of trying to patch up a broken model.

The key thing about async changes (and that relates to error boundaries) is that the rendering phase (componentWillMount, render) can be paused, resumed, aborted, restarted from scratch, etc. Only the commit phase (componentDidMount, componentDidUpdate, componentWillUnmount) always happens when we’re sure what’s going on. Any side effects should be done in the commit phase. They are done in the commit phase now too, but the guarantees about componentWillMount order are no longer truly there which is why we have this issue.

I’m not sure how to solve this yet. But we’ll need to figure this out. It may be representative of a broader pattern people might be relying on in another projects too.

We forked react-side-effect and create https://github.com/openameba/react-reffect.

// BodyStyle.js
import { Component, Children } from 'react';
import PropTypes from 'prop-types';
import createSideEffect from 'react-reffect';

class BodyStyle extends Component {
  render() {
    return Children.only(this.props.children);
  }
}

BodyStyle.propTypes = {
  style: PropTypes.object.isRequired
};

function reducePropsToState(propsList) {
  var style = {};
  propsList.forEach(function (props) {
    Object.assign(style, props.style);
  });
  return style;
}

function handleStateChangeOnClient(style) {
  Object.assign(document.body.style, style);
}

export default createSideEffect(
  reducePropsToState,
  handleStateChangeOnClient
)(BodyStyle);

const store = BodyStyle.createStore();
return (
  <BodyStyle.Provider store={store}>
    <BodyStyle.Consumer style={{ backgroundColor: 'red' }}>
      {this.state.something ? <SomeComponent /> : <OtherComponent />}
    </BodyStyle.Consumer>
  </BodyStyle.Provider>
);

We haven’t made a PR to react-side-effect just because this library is not using HOC.

Hey, I was looking for some solution to nest document.titles that would support API as:

<Titled title={() => 'Example.com'}>
  <h1>Welcome!</h1>
  <Titled title={title => `Homepage | ${title}`} />
</Titled>;

that outputs

Homepage | Example.com

but is seems that react-helmet or react-document-title (react-side-effect) always do “the deepest one wins” without possibility to compose upstream titles.

So inspired by this thread (great read btw!) I just put together a library react-titled. It uses the new context API to pass around two functions:

• updateParent to recursively reach all upstream instances until reaching the top one that will reduce all titles and set the document.title.
• resetHeight, the longest tree path should win so I need to track depth at every instance; however, once componentWillUnmount is triggered somewhere I need to reset all heights and recompute

I’m actually quite happy how simple the code is. Two caveats:

• I am not sure how to go about SSR (I don’t really need it for my use-case). I suppose you could pass a function into top Titled onChange and obtain the title that way. But componentDidMount / componentDidUpdate are not called with renderToString right? What’s the recommendation for SSR in React17 world?
• Siblings. Generally, I assume there are never two siblings mounted at the same time (not sure what would be the use-case?). However, if that happens, the last sibling wins and document.title is set multiple times (with every sibling). No big deal to me.

I whipped up a little proof-of-concept using React’s new Context API. The code isn’t that clean (lots of repetition) but it seems to work.

It works by registering each instance after it mounts to its parent’s state which is found through context. This bubbles up until it reaches the Provider which then reduces the props of the whole tree. Updates are propagated via incrementing the parent’s count state variable (hacky?) on componentDidUpdate until it reaches the Provider.

I haven’t even thought about SSR at this point. Also peek() no longer works.

Usage:

const DocumentTitle = withSideEffect(/* ... */) // as normal

function render() {
  return (
    <DocumentTitle.Provider> // this is now required
      <DocumentTitle title="Outer">
        <DocumentTitle title="Inner" />
      </DocumentTitle>
    </DocumentTitle.Provider>
  );
}

@gaearon Let me know if I’m on the right track.

cc @bvaughn @lourd

Since we already branch on whether we have the DOM, it seems acceptable to use the same check to push instances in render (if we’re rendering on SSR).

So, I investigated fixing this and I’m not sure how to solve this in a nice way. I did come up with a hacky solution but it will not work nicely with React’s upcomingAsyncMode.

I tried the componentDidCatch idea but it caused any error to look like it was coming from react-side-effect and not the actual component that had an error which made debugging annoying.

Moving the code to componentDidMount fixes issues with errors but the order of the mounted instances are incorrect and inconsistent.

The “working” hacky solution works by splitting the logic from componentWillMount into a two phases. When the component mounts it is added to the mountedInstances array but marked as pending. A 0ms timer with a callback to remove the component is also set up. Then in componentDidMount the component is marked asresolved, the timer is cleared and emitChange is called. emitChange is changed to only reduce props for components that are in the resolved state.

The timeout is necessary because as far as I know there is no way of telling if the component failed to mount without implementing componentDidCatch and losing the error’s context. I think the same approach could work in AsyncMode but the timeout would have to be longer. (1 second?)

It would be cool if we could get input from some of the React core devs as they will have more insight into how to solve this in a nicer way.