Reference's onInvalidated() called after map.put(snapshot) when snapshot doesn't have an id

[aplum]

Bug report

• I've checked documentation and searched for existing issues
• I've made sure my project is based on the latest MST version
• Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code
https://codesandbox.io/s/mobx-state-tree-issue-1874-gl01j

I've just started using MobX and MST to replace (a very poor implementation of) Redux in my app, and I came across some behaviour that seems like a bug. The closest related discussions I've found are here and maybe here.

The linked codesandbox has a similar structure to part of my app. It has Users and Todos. Todos have an array of assignedUsers safeReferences, and a primaryAssignedUser reference (this reference will also exist in the array). The array will never be empty, and primaryAssignedUser will always exist.

Models:

export const User = types.model("User", {
  id: types.optional(types.identifier, () => "" + Math.random()),
  name: types.string
});

let nextTodoId = 1;
export function todoIdGenerator() {
  return `t${nextTodoId++}`;
}

export const config = { skipOnInvalidated: false };

export const Todo = types.model("Todo", {
  id: types.optional(types.identifier, todoIdGenerator),
  title: types.string,
  finished: false,
  assignedUsers: types.array(
    types.safeReference(User, {
      acceptsUndefined: false,
      onInvalidated() {
        console.log("assignedUsers onInvalidated called");
      }
    })
  ),
  primaryAssignedUser: types.reference(User, {
    onInvalidated(ev) {
      console.log("primaryAssignedUser onInvalidated called:", ev);
      if (config.skipOnInvalidated) {
        console.log("primaryAssignedUser onInvalidated logic skipped");
        return;
      }
      const nextRef = ev.parent.assignedUsers.find(
        (u) => u.id !== ev.invalidId
      );
      if (!nextRef)
        throw new Error("A Todo should never have zero assignedUsers");
      ev.replaceRef(nextRef);
    }
  })
});

export const TodoStore = types
  .model("TodoStore", {
    todos: types.map(Todo),
    users: types.map(User)
  })
  .actions((self) => ({
    addTodo(todoSnapshot) {
      self.todos.put(todoSnapshot);
    }
  }));

index.js:

const store = TodoStore.create({
  users: {
    u1: { id: "u1", name: "Alice" },
    u2: { id: "u2", name: "Bob" },
    u3: { id: "u3", name: "Charlie" }
  }
});

render(<TodoList todoStore={store} />, document.getElementById("root"));

console.log("Adding first todo");
store.addTodo({
  id: todoIdGenerator(),
  title: "Get coffee",
  assignedUsers: ["u1", "u2"],
  primaryAssignedUser: "u2"
});
console.log("First todo added. Todos:", getSnapshot(store.todos));

/*
Uncomment either LINE_A or LINE_B to avoid an error being thrown.
If LINE_A is uncommented, the array of assignedUsers will be empty.
  - If assignedUsers is configured as `types.reference` instead of `safeReference`, the Todo instance is created as expected.
If LINE_B is uncommented, the Todo instance is created as expected.
*/

// config.skipOnInvalidated = true; // LINE_A,
console.log("Adding second todo");
store.addTodo({
  // id: todoIdGenerator(), // LINE_B.
  title: "Get more coffee",
  assignedUsers: ["u2", "u3"],
  primaryAssignedUser: "u2"
});
console.log("Second todo added. Todos: ", getSnapshot(store.todos));

Describe the expected behavior
Regardless of whether an id is provided in the snapshot, onInvalidated() should not be called.

Describe the observed behavior
As indicated in the comments above (about LINE_A/LINE_B), everything works fine if the provided Todo snapshot already has an id. However, if I want to omit the id (since it's optional in the model):

• onInvalidated() gets called for each reference.
• The array will be empty. More generally, any safeReferences will be undefined/removed.

If I wasn't using onInvalidated() (manually or via safeReference), I wouldn't even notice the extra steps happening when omitting the id. And in my case, the ids are all crypto.randomUUID(), so working around this is just the slight inconvenience of manually adding the ids. This tripped me up for a while though, and I'm not sure whether it's a bug, or undocumented but expected behaviour – similar to how calling map.put(Todo.create({...})) would understandably result in invalid references, since at Todo.create({...}) the model instance is not yet part of the tree.

[coolsoftwaretyler]

Hey @aplum - thank you for the detailed write up, and I'm sorry for the wait time on getting a response.

I've played around with the sandbox you put together, and I'm inclined to label this as a bug. I think it should be straightforward for us to write some tests, validate the problem, hopefully resolve it, and update documentation as such.

I'm a little new to the project, so there's still a chance this is expected behavior, in which case we ought to update the documentation about it, because I don't think this is clear. We might even write some tests as well to demonstrate that it happens and codify that it is intended to do so.

All that said, not sure where it will land on the priorities. I know it's already been a long time and you mentioned having a workaround. I'll leave this open and hopefully we can resolve it for ya.

PR definitely welcome if you'd like to pitch in, but I totally understand if you have moved on from the problem. Thanks!

[longnguyen2508]

I had the same issue as well. :(

[coolsoftwaretyler]

Thanks for the confirmation @longnguyen2508 - sorry for the inconvenience! Hopefully we'll get to this soon, but I'd be happy to review a PR as well.