Reference changes without reason

[eddiecooro]

Hi.
Thank you for this impressive library.
I'm working on a project which is heavily relied on redux-orm for its state management and logic.

Recently, I had many issues about poor performance and lags, especially in long lists. So I started monitoring the bottlenecks and re-architecture the application. After cleaning up everything, I found this issue which makes many re-renders that are not needed.

So, here is what I have:
Consider having two models, FakeModelOne and FakeModelTwo such that FakeModelOne has many-to-many(or many-to-one) relation with FakeModelTwo. Now if we write a reducer which adds items to FakeModelOnes related field (Using QuerySet.add(...)), in the next iteration when we get every related instance of FakeModelTwo. The reference of old related instances has changed and thus, every component re-renders although they are using memo.

Here are some code samples for clarification:
many-to-many
many-to-one

In both of these examples, we have a component which lists the related models, and when you press the button, a new model gets generated and added to the list, but if you take a look at the console, you will find out that every component in the list gets re-rendered, but there is no value change in their content.

[haveyaseen]

First of all, the toModelArray() calls in your selectors do need to be toRefArray(). Model instances will always get new references during toModelArray() while references will only ever change if you call instance.update() with new props (which is also called in Model.upsert() if a ref with the same ID already exists).

That being said, I'm not sure why but it seems that PureComponent fails to prevent re-renders in this case. If you replace ModelOneItem by a functional component wrapped in a React.memo() call it works as intended:

const ModelOneItem = React.memo(function ModelOneItem({ item }) {
  console.log(item);
  console.count("ModelOneItemRender");
  return <pre>{JSON.stringify(item)}</pre>;
});

[eddiecooro]

Thank you so much for your response.
I've tried both React.memo and toRefArray, but I never tried them both at the same time 😂
Yes, it works as expected, but as I understand, now we have two options
First, using the toModelArray directly and dealing with the renders (maybe using deep-equal for equality check) which probably will not be efficient at all,
Second, using toRefArray method, but losing every relationship that the instance has. But our component needs those data to render, so we can do either of these things (I consider the relation is oneToOne for the sake of brevity):

1- parsing the needed fields by ourselves:

const friends = ModelOne.withId(1).friends.toRefArray().map(f => { ...f, room: Room.withId(f.room) })

which is not efficient by any mean 😂

2- parsing fields separately in the selector:

mapStateToProps = (state) => {
    ...
    const data = session.X.all().toRefArray();
    return {
        data: data,
        friends: data.map(d => session.Friends.withId(d.friend)),
    }
}

which preserves reference on the data, but seems too verbose to me.
Or alternatively:

mapStateToProps = (state) => {
    ...
    const dataQuerySet = session.X.all();
    const friends = dataQuerySet.map(d => d.friend).toRefArray();
    return {
        data: dataQuerySet.toRefArray(),
        friends: friends
    }
}

But with my knowledge of the codebase, both of these will almost have the same performance.

So, do you think it's possible to keep the reference of the toModelArray result consistent?
I am even willing to contribute to the codebase if you give me the direction.

[haveyaseen]

Yeah, I was also very surprised to see that it didn't work with PureComponent but using toRefArray() 😂 It's interesting that @ernsheong has made similar points in #253 recently. The root of the issue I think lies in the fact that the Model class is a very intuitive place for custom logic which needs to be exposed in a DRY way to the view layer.

The idea of combining the ref and model concepts looks less and less elegant to me, though. Tommi himself has also said the following in #58 (comment):

I think keeping item.ref as a simple reference to the plain object in the store is important; it keeps selectors fast.

If you're duplicating that logic in several selectors, you could implement a method on the Model to return an object that gets the values from methods you wish (e.g. toJS). A simple standalone function that takes a Model and returns it would work too.

I have done it using a custom conversion function myself, as suggested in #180 (comment) as well. However that does create a new reference on each call which is arguably bad for performance.

Values derived from other properties are highly useful in the view layer but we should probably not make sessions available there to interact with other models. In Redux data is not supposed to flow back from the view, other than by dispatching actions. Exposing sessions and models would enable that which would be encouraging a terrible practice [1].

---

Now, to allow for computed properties, I imagine we could define derived properties using the attr() field in Model.fields (see also #62). They would be copied over mutably to the refs upon writing to the DB in Model.create and Model#update.

To avoid bugs, we might want to prevent setting those fields within Model.create and Model#update. Or even better, the definition within attr() can be considered a default value only – or a function to compute the default value – in case it is not set by the user themselves.

The Model needs to have some function to know which fields of its row/ref it needs to fill up with the value from attr(). The function would need to mutate the ref. attr() fields may also need to be disregarded when performing equality checks. It's probably best to require them to be functions. That way they can receive parameters and are pretty much guaranteed to be referentially equal, preventing bugs when users make them contain non-primitive values.

You are very welcome to contribute 🙂 🙂 Please let me know what you think of my suggestions, if you're still interested in this and if you need any further direction!

---

[1] Values computed using relationships should stay in selectors. With #257 this will be way easier, so we can soon do the following:

const xs = createSelector(orm.X);

// For a component listing all X models:
mapStateToProps = (state) => ({
  xs: xs(state),
});
const XList = connect(mapStateToProps)(({ xs }) => xs.map(
    x => <XFriends xId={x.id} />
));

// For a component listing a single X's friends:
// in XList: <XFriends xId={x.id} />
const xsFriends = createSelector(orm.X.friends);
const friendRoom = createSelector(orm.Friend.room);
const xsFriendRooms = createSelector(orm.X.friends.map(friendRoom));
mapStateToProps = (state, ownProps) => ({
  friends: xsFriends(state, ownProps.xId),
  friendRoom: xsFriendRoom(state, ownProps.xId),
});
const XFriends = connect(mapStateToProps)(({ friends, friendRoom }) => friends.map(
    (friend, i) => <div>{friend.name} - {friendRoom[i].location}</div>
));

// Or a much cleaner component listing a single friend instead:
// in XFriends.render(): <Friend id={friend.id} />
const friends = createSelector(orm.X.friends);
mapStateToProps = (state, ownProps) => ({
  friend: friends(state, ownProps.id),
  room: friendRoom(state, ownProps.id),
});
const Friend = connect(mapStateToProps)(({ friend, room }) => (
    <div>{friend.name} - {room.location}</div>
));

[eddiecooro]

I can't wait for the new selectors API. It seems so nice and tidy.
So what I understood is that by using model relationships outside of the selectors, I'm swimming against the redux-orm 😄
And the best thing we can do right now is writing separate selectors for each relation we need (because resolving relationships in the root selector creates new objects and causes unnecessary re-renders)
Although it's not practical for us to use just React.memo in our project, because we still don't have hooks and class components are needed. So I will investigate the issue with the PureComponent further and let you know when I find something.

[eddiecooro]

Ok. I found the problem with the pure component 😆
I made a silly mistake!!!
I wrote PureComponent but the imported component was { Component as PureComponent } 😑 😑
Sorry about the mistake and thank you so much for your guides ❤️
By the way, do you have any clue when we can use redux-orm with the new selectors API?

[haveyaseen]

Didn't notice that either hah! Looks to only be the case for the many-to-many example but it's probably something similar in the other one.

As for the new API, I just released a version 0.14.0-rc.0 for you to try out 🕺

[eddiecooro]

Thank you so much for the release, I'm going to try it out. Also, about the computed properties, they are so needed. Currently, I'm going to write custom selectors for calculating properties based on other fields. But I'll work on your suggestion about adding support for computed properties to attr fields when I find the time.

[haveyaseen]

Meanwhile computed properties can be emulated this way:

class Rectangle extends Model {
    // ...

    /**
     * Act as if this method was a non-static function.
     * It's only static because it needs to be accessible during Rectangle.create.
     * `this` will be bound to the ref automatically.
     */
    static area() {
        return this.width * this.height;
    }

    static create(props) {
        Model.create.call(this, {
            ...props,
            area: this.area,
        });
    }
}

In view (e.g. React):

function RectangleComponent({ rectangle }) {
    return (
        <div>
            <span>Area: {rectangle.area()}</span>
            <span>({rectangle.width} x {rectangle.height})</span>
        </div>
    );
}

[ernsheong]

Feels like a hack, using static as if non-static. I hope we can still consider merging refs and models with the help of Proxy (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy)

[haveyaseen]

Yes it is a hack but it works for now.

While using Proxyis an interesting idea unfortunately it's not transpilable to ES5 nor polyfillable. Using it would mean dropping support for IE11 and a few other browsers which I don't think we should do.

[ernsheong]

There's https://github.com/GoogleChrome/proxy-polyfill, probably imperfect depending on the use case.

As for IE11 etc., a major version bump could help everyone to progress, but that's just my 2 cents as a non-maintainer.