Prepare for react async rendering

[daviderenger]

React 16.3 introduce some new lifecycle hooks and they will deprecate some of the old one in the next release. Also they are very clear with that the constructor, componentWillMount and some others must be side effect free.

http://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/

Currently in MVx uses the constructor and componentWillMount for setting up stuff, like state and starts listen to props. That will lead to leaks later on when react decides to cancel a job and never mount the component at all (componentWillUnmount will never be called). React suggests that all code that add listeners that is supposed to be cleaned out in componentWillUnmount should be added in componentDidMount.

[gaperton]

That's really bad.

[gaperton]

At the first glance, it looks like it makes the custom state management impossible.

[gaperton]

1. State must be initialized before the render in the constructor. The lack of the destructor in new React will cause memory leaks in some cases unless the new Events engine will be implemented using the WeakMap. The same for the Store.

2. Events subscription happens in "did" methods, so it's fine.

3. The real problem starts with a removal of componentWillReceiveProps and inability to modify state before the render. props watchers and state synchronization won't work as expected, to be exact. I hope it can be handled somehow.

[gaperton]

Nah, it's even worse. In case of the WeakMap keys are weak and there's no iteration support. Thus, there seems to be no obvious way to use it as an actual weak reference storage.

Therefore, (1) is unsolvable unless there will be the way to delay the event subscription for the record/collection tree to some moment of time after the aggregation tree is created (special { delayEventsSubscription } constructor option is needed, + the method to subscribe for changes recursively). Could be done in the Events level (the listener's operation mode when listenTo doesn't actually create the back reference at the source, + the method to make sure events are enabled.

To solve (3), all updates to the state made in the lifecycle hooks must be muted. Well, it can be done.

[daviderenger]

I think componentWillReceiveProps still can be used if its only is run after component is mounted and if it only updates listeners that already exists it should be fine.

But there are more problems with the current form of MVx, for example the lastest version of react-hot-loader is not working. I'm looking into replacing it with a much simpler solution that only implements listenToProps and completely skip the state replacement. I have noticed that its problematic for fresh programmers to use setState in some components and state.xx = in others.

Also we are only using listenToProps so the lib is overkill. I did some measurments for the initializing of a component with pure react (0.1ms) and MVx (5.0ms). With my solution below the result was about 0.2ms.

So this is a first prototype that does what I want.:

import { forEach } from 'lodash';


function addListener(listensFor, name, modelOrCollection, handler, map) {
  let active = false;
  if (modelOrCollection && !!modelOrCollection.on) {
    try {
      modelOrCollection.on(listensFor, handler);
      active = true;
    } catch (e) {
      console.error('listenToProps Failed to listenToProp, not a model or collection?', e);
    }
  }
  map.set(name, {
    active,
    listensFor,
    modelOrCollection
  });
}

const listenToProps = inProps => target => {
  return class extends target {
    componentDidMount() {
      this.mounted = true;
      //console.log('listenToProps componentDidMount extended', this);
      this.forceIt = model => {
        console.log('listenToProps trigger rerender', model, model.changed);
        this.forceUpdate();
      };
      this.propsListensToMap = new Map();
      forEach(inProps, (listensFor, name) => {
        const modelOrCollection = this.props[name];
        addListener(listensFor, name, modelOrCollection, this.forceIt, this.propsListensToMap);
      });

      super.componentDidMount && super.componentDidMount();
    }

    UNSAFE_componentWillReceiveProps(nextProps) {
      //console.log('listenToProps unsafe_componentWillReceiveProps');
      if (this.mounted) {
        for (const [name, { active, listensFor, modelOrCollection }] of this.propsListensToMap) {
          if (this.props[name] !== nextProps[name]) {
            //console.log('listenToProps prop changed, remove old listeners and add new');
            if (active) {
              //console.log('listenToProps was listening, removes the old listeners');
              modelOrCollection.off(listensFor, this.forceIt);
            }

            addListener(listensFor, name, nextProps[name], this.forceIt, this.propsListensToMap);
          }
        }
        // todo, check each prop if it changed and remove/apply listeners
      }
      super.UNSAFE_componentWillReceiveProps && super.UNSAFE_componentWillReceiveProps();
    }

    componentWillUnmount() {
      for (const [name, { active, listensFor, modelOrCollection }] of this.propsListensToMap) {
        //console.log('listenToProps unmount', name, active, listensFor, modelOrCollection);
        try {
          if (active) {
            modelOrCollection.off(listensFor, this.forceIt);
          }
        } catch (e) {
          console.error('listenToProps Failed to stop listenToProp', e);
        }
      }
      super.componentWillUnmount && super.componentWillUnmount();
    }
  };
};

export default listenToProps;


/*
@listenToProps({
  me: 'change:remote',
  myAdmin: 'change:powerUpCalendar change:powerUpEmail'
})
export default class MainActionButton extends PureComponent {
  static defaultProps = {
    me: null,
    myAdmin: null,
    small: false,
    searchOpen: false,
    numberOfSmallDetailViews: 0,
    numberOfMinimizedDialogs: 0,
    tabsVisible: false,
    toastVisible: false
  };

  */

(edited so listeners map is not shared between instances)

[gaperton]

I have noticed that its problematic for fresh programmers to use setState in some components and state.xx = in others.

Well. That can be solved rather easily. Look at that commit. But we at VDMS/Volicon don't use setState at all. There's no advantage in doing that.

60671f7

This setState is still better than standard. For instance, this statement will deeply merge the state's a member, instead of just overwriting it.

this.setState({ a : { b : 1 }});

The biggest problems of standard state is that you can't easily pass its part down as "mutable prop" (well, you can with the help of our NestedLink lib), and that you can't easily reuse parts of the complex state (even with NestedLink).

[gaperton]

Also we are only using listenToProps so the lib is overkill. I did some measurments for the initializing of a component with pure react (0.1ms) and MVx (5.0ms). With my solution below the result was about 0.2ms.

If you want local component updates on external state changes, it can be done like that.

static state = {
    items : someGloballyAvailableRecordOrCollectionInstance
}

Passing props is not really required.

[gaperton]

Also we are only using listenToProps so the lib is overkill. I did some measurements for the initializing of a component with pure react (0.1ms) and MVx (5.0ms).

That's quite sad, but I never saw it in the profiler's top in our application. First of all, the Component has no lifecycle hooks defined by itself, so there should be zero overhead. When you start using features like state or props, it will dynamically construct the mixin with the required features only. For the case of the props change listener, it's this:

const ChangeHandlersMixin = {
    componentDidMount(){
        handlePropsChanges( this, {}, this.props );
    },

    componentDidUpdate( prev ){
        handlePropsChanges( this, prev, this.props );
    },

    componentWillUnmount(){
        handlePropsChanges( this, this.props, {} );
    }
};

function handlePropsChanges( component : any, prev : object, next : object ){
    const { _changeHandlers } = component;
    
    for( let name in _changeHandlers ){
        if( prev[ name ] !== next[ name ] ){
            for( let handler of _changeHandlers[ name ] ){
                handler( next[ name ], prev[ name ], component );
            }
        }
    }
}

And the handler itself looks like this:

function( next, prev, component : any ){
    prev && component.stopListening( prev );
    next && component.listenTo( next, changeEvents || next._changeEventName, component.asyncUpdate );
}

That's all you'll get in your component when using just props listeners, and as you can see, there is no extra code there. To optimize it, however, you should avoid stopListening/listenTo calls in favor of on/off, and avoid for-in loops. The reasons it's not optimized yet is that React-MVx is designed with different architectural patterns in mind.

• We don't use singletons, preferring the local state component and local stores at the page root level.
• We don't use local updates in mobx style, but top to the bottom updates with pure render in a similar way as redux does.
• Unlike redux, we use state synchronization with props watchers to avoid rendering the whole world on every key press in some form (someProp : Record.has.watcher( assignToState( 'smateMember' ), and then the form elements are bound to the local state so UI updates will naturally be local). That's the "no-no" pattern for them, and "sure, you're welcome" for us. Declarative and easy, and it gives huge performance gain. There's no point to simulate the local state in the "single source of truth" when you can just have it naturaly.
• The majority of our components are purely functional components (defined as functions). We use classes only if we need state or pure render (which is enabled with a single line). Class components are really less than 10% of all the code and tend to be at the top of the UI component tree.

Because of that reasons, there is practically no any noticeable overhead, and I have to note that our app has really complex UI operating with large data sets of thousands of items.

[gaperton]

So, React-MVx is written to support that architectural approach I briefly described above.

If you prefer to go with mobx style (singletons + local events subscription), you don't really need ReactMVx Component. The simple decorator will do (as you described), and there is really nothing magical about the way how it's done. It will save you few KBs.

[gaperton]

But I don't like mobx style. It's memory hungry, too magical thus it's easy to make a mistake, and there are no real-world performance problems which can justify that as an approach of the first choice even for the complex UI case.

I'd say all the problems really start with a "single source of truth" and "state management is hard" mantras and singletons. They want it to be hard, so they made it to be hard.

[daviderenger]

Hi again, I'm sorry that this issue ended up as an architectural discussion, it was not my intent! I'm not trying to say that MVx is bad or anything either, we rely heavily on it and type-r!

I dont think our architectural approach is so much different from yours, this is basically what we do:

For example a chat room we have some kind of root element that holds the collection of chat messages, responsible for fetching more and so on. It holds the collection in its state so its updated on changes. Then it renders children and passing down chat message models (records) as props. The chat message component has a lot of different children that renders different parts but as we pass along the model the leafs can care for and rerender only on changes that are relevant. No where are we updating the actual model from within one of these leaf but all actions are sent directly to the server.

So with MVx we dont have to spread out all props and pass them along, that is convenient and also only the relevant components are rerendered and not all parents in between the "root" and the leaf; great for performance.

What I'm saying is not there are some things in MVx that we dont use and others we do use that can be worked around (putting something in a state for changes can be handled manually with on/off etc). And since MVx is conflicting with some of the inner parts of React it might be wise to look over the implementation without changing any Reacts internals, like the state handling for example. I dont really know why its not working with the new react-hot-loader but it complains and fails on some react.prototype manipulation. Recreating the great stuff that MVx offers might be possible to do without the "enhancing" parts!

And back to the original topic! It will probably break some code for some but moving all init code that depends on being cleaned up by componentWillUnmount to componentDidMount seems possible. componentWillReceiveProps can still be used but make sure it only add listeners and such after component is really mounted (looks like it should be fine as it is now?)

[gaperton]

Hi again, I'm sorry that this issue ended up as an architectural discussion, it was not my intent! I'm not trying to say that MVx is bad or anything either, we rely heavily on it and type-r!

Please. It's wonderful to have architecture discussions. I don't have an idea why it became sorta tabu in the community.

Btw, there are troubles with stores API as well. Need to be redesigned as they are changing the context API. #12

For example a chat room we have some kind of root element that holds the collection of chat messages, responsible for fetching more and so on. It holds the collection in its state so its updated on changes. Then it renders children and passing down chat message models (records) as props. The chat message component has a lot of different children that renders different parts but as we pass along the model the leafs can care for and rerender only on changes that are relevant. No where are we updating the actual model from within one of these leaf but all actions are sent directly to the server.

In this case, you have to take some additional actions to prevent the double render, because the whole component tree will be rendered anyway on the root state changes. If you use static pureRender = true declaration in children it will happen automatically as long as you mentioned all the relevant props in the static props declaration, and you don't need to subscribe for the changes in this case.

You might have other reasons to use local change subscription, though. Not sure I understood it well.

And since MVx is conflicting with some of the inner parts of React it might be wise to look over the implementation without changing any Reacts internals, like the state handling for example.

Well, the single thing we do is overriding the state member. React core is not touched, it's done in a subclass and React namespace is not modified but rather extended. It's not so terrible.

What I can do though is to move it to the StatefulComponent base class instead of overriding Component. I think it's a good thing to do.

[gaperton]

And back to the original topic! It will probably break some code for some but moving all init code that depends on being cleaned up by componentWillUnmount to componentDidMount seems possible. componentWillReceiveProps can still be used but make sure it only add listeners and such after component is really mounted (looks like it should be fine as it is now?)

Yep, even now the majority of event subscription is being handled in componentDidMount(), so it shouldn't be an issue.

componentWillReceiveProps() is always being called when the component is mounted. The problem is that they are going to deprecate componentWillReceiveProps() in future, and the new getDerivedStateFromProps() lifecycle hook is static (what the hell). Which leave us no other options than try to override the state putting there the reference to the component (state Record already has such a reference as state._owner).

It the worst case, if it will seriously break something inside of the React we will have to move the record state to the different member like this.model instead of making the state override while putting the reference to the component into the state (this.state = { _component : this }).

I have to admit that the React team actually works really hard to make custom state management solutions impossible. They may even succeed once, but not this time. :)

[gaperton]

And speaking of the standard React state - it has no any single advantage over the Type-R record. It shouldn't be even faster to access and manipulate with. The record uses the real class for its attributes, which is being optimized by JIT as a real structure, not the hash.