Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating related fields from memoized selector #218

Closed
talolard opened this issue Jul 3, 2018 · 8 comments
Closed

Updating related fields from memoized selector #218

talolard opened this issue Jul 3, 2018 · 8 comments

Comments

@talolard
Copy link

talolard commented Jul 3, 2018

Hi,
We're super happy to see this project moving again! We're still a few versions back, and ran into an issue.
Anyway, we have model A and model B. B has a foreign key to A.

At some point we call an instance of A and dispatch an action to add new B's to it.

We're getting A via a memoized selector, and it looks like it doesn't get an indication that a B was added, so A.bs.toModelArray doesn't update with the B that was added.

I couldn't tell if this is a known issue, by design or something resolved recently.

We'd like to know if the newer versions address this or is their something we can do
Thanks

@talolard
Copy link
Author

talolard commented Jul 3, 2018

So I ended up with having a parent listen for the creayion of B,C,D and update itself with timestamp.
This seems hackish and wrong, am I on the right track ?

class A extends Model {
  static reducer(action, A, session) {
    switch (action.type) {
      case CREATE_A: {

        A.create({ action.payload });
        break;
      }
      case CREATE_B:
      case CREATE_C:
      case CREATE_D:

        const id = action.payload.a_id; // The id of a is in the payload for B,C,D
        A.withId(id).update({"touched": new Date()})

      default:
    }
  }
}

@haveyaseen
Copy link
Member

haveyaseen commented Jul 3, 2018

If I understand your use-case correctly, this should not be necessary at all. What are your A.fields and B.fields? And how does the selector look like? If a is an instance of A and you access a.bs within the selector, then your selector should be called again if you create/update/delete an instance of B.

@talolard
Copy link
Author

talolard commented Jul 4, 2018

Thanks.
I'll switch from A and B to the actual code.
I have two models, Project and Datset and a Dataset has a FK to a Project

class Project extends Model {
  toString() {
    return `Project: ${this.id}`;
  }
  static reducer(action, Project, session) {
    switch (action.type) {
      case RECEIVE_PROJECT: {
        const { id, name } = action.payload;
        Project.create({ id, name });
        break;
      }

      default:
        break;
    }
  }
}
Project.modelName = 'Project';
Project.fields = {
  id: attr(),
  name: attr(),
  touched: attr()
};

The dataset

class Dataset extends Model {
  toString() {
    return `Dataset: ${this.id}`;
  }

  static reducer(action, Dataset, session) {
    switch (action.type) {
      case CREATE_DATASET: {
        Dataset.upsert(action.payload);
        break;
      }
      default:
        return undefined;
    }
  }
}
Dataset.modelName = 'Dataset';
Dataset.fields = {
  id: attr(),
  name: attr(),
  id_field: attr(),
  aggregation_field: attr(),
  order_field: attr(),
  content_field: attr(),
  project: fk('Project', 'datasets')
};

The project selector

const _projectSelector = createSelector(
  orm,
  state => state.orm,
  session => {
    return session.Project.all().toModelArray();
  }
);

And then, in a connected component that uses that selector (and takes the first element in the array) I have

        {props.project.datasets
          .toModelArray()
          .map(dataset => (
            <ListGroupItem key={dataset.id}>{dataset.name}</ListGroupItem>
          ))}

or more simply this
{props.project.datasets.count()}

Neither of which updates when I add a new dataset.

I've confirmed the dataset is updated with the proper id of the project, and the project exists in the orm with that id.

Thanks

@haveyaseen
Copy link
Member

haveyaseen commented Jul 4, 2018

Using models and relationship accessors outside of selectors is not supported by design. In this case specifically you are first executing the selector and accessing project.datasets afterwards. Redux-ORM has no way of knowing that you care about project.datasets so your _projectSelector won't be re-executed when you are performing changes on the Dataset model only.

Please use session.Project.all().toRefArray() and session.Dataset.all().toRefArray() instead. For convenience, you could also create a different selector that returns the datasets for each project as raw references:

const _projectDatasets = createSelector(
  orm,
  state => state.orm,
  session => session.Project.toModelArray().reduce((map, project) => ({
    ...map,
    [project.id]: project.datasets.toRefArray(),
  }), {})
);

And then in your components, {props.projectDatasets[props.project.id].length} and

        {props.projectDatasets[props.project.id]
          .map(dataset => (
            <ListGroupItem key={dataset.id}>{dataset.name}</ListGroupItem>
          ))}

@talolard
Copy link
Author

talolard commented Jul 4, 2018

Oh that's elegant. Thanks.
As an aside, we've been doing it the wrong way in production for about 6 months and it worked fine until now (when we turne don memoization). The convenience of having a .datasets() on a project (and similar concepts) has been huge.

@haveyaseen
Copy link
Member

haveyaseen commented Apr 15, 2019

Closing in favor of #253.

@eddiecooro
Copy link

eddiecooro commented May 11, 2019

Hi, I wonder if we can reopen this issue.
I have a situation that may be related to this one.
Consider having a selector which returns the First Model named Rooms, and another selector which uses this selector as a dependency and returns all messages of that specific room. So we can write something like this:

const getRoomID = (state, props) => props.roomID;
const getRoom = createSelector(orm, dbSelector, getRoomID, (session, roomID) => {
  const room = session.Rooms.withId(roomID);
  return room; 
});
const getRoomMessages = createSelector(orm, dbSelector, getRoom, (session, room) => room.messages.all().toRefArray() );

But the problem is that either room or data may not exist and we want our app to be fault-tolerant. So we should return a default case when either of them doesn't exist.

const getRoom = createSelector(..., (session, roomID) => {
  return session.Rooms.withId(roomID) || {};
});
const getRoomMessages = createSelector(..., (session, room) => room.messages ? room.messages.all().toRefArray() : [] );

But now if the room doesn't exist the first time we use the getRoomMessages selector, room.messages.all().toRefArray() never gets executed, and even if we add new messages to the room messages field, the result of getRoomsMessages always will be an empty array.
It would be nice if when the user queries room.messages, redux-orm could mark it as accessed and whenever a new model related to the room gets created, the selector executes again.

@haveyaseen
Copy link
Member

haveyaseen commented May 12, 2019

@Eddie-CooRo The problem here, again, is returning models from selectors. Because this happens so often I'm starting to think that we should actually forbid returning model instances by throwing an error.

Selectors were not composable the way you did it. You would have had to call session.Rooms.withId() within getRoomMessages. Then both room.ref and room.messages get marked as accessed as you described (conceptually). Now, you no longer need to do that as v0.14.0-rc.0 accounts for this 🙂

const getRoom = createSelector(orm.Rooms);
const getRoomMessages = createSelector(orm.Rooms.messages);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants