Model Creation/Instantiation Allows Duplicate names (except for properties)

[coolsoftwaretyler]

Bug report

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

Sandbox link or minimal reproduction code

https://codesandbox.io/p/sandbox/duplicate-views-no-error-92z9c7

Describe the expected behavior

In this code:

import { getSnapshot, t } from "mobx-state-tree";

(async function () {
  const Person = t
    .model("Person", {
      name: t.string,
    })
    .actions((self) => ({
      anAction() {
        self.name = "Changed";
      },
    }))
    .views((self) => ({
      something() {
        return "something";
      },
      something() {
        return "something else";
      },
      anAction() {
        return "Ooh that was weird";
      },
    }));

  const you = Person.create({
    name: "your name",
  });

  const snapshot = getSnapshot(you);

  console.log(snapshot);

  you.anAction(); // It looks like this is *not* changing the name, so the second declaration overwrites the first.
  console.log(you);
  console.log(you.something()); // This gets the latest declaration value, 'something else'
})();

Notice that we encounter no runtime errors on the duplicate names. In some cases, like the duplicate view names, some editors may report a redline about the duplicate. But that does not seem to happen across actions and views method calls.

We noticed this in https://github.com/mobxjs/mobx-state-tree/pull/2207#pullrequestreview-2243495912, because we were moving some of the properties duplicate checks into the views and actions methods, and I was expecting to see other duplicate checks, but none exist.

What should happen?

If a user defines a model with duplicate keys, instead of overwriting the prior keys, we should throw a helpful error that says something like "[keyname] exists already in [views | actions | volatile]", much like we do when users duplicate a property on the model itself.

What happens instead?

No runtime errors, and the most recently defined property overwrites the least recently defined properties.

[coolsoftwaretyler]

@thegedge - I think we just need to make sure we don't break types.compose. For a trivial instance, a loud person model yells, a quiet person model whispers. I compose to a regular person and expect the second model to "override" the first (almost simulating a type of inheritance, I suppose):

import { t } from "mobx-state-tree";

(async function () {
  const LoudPerson = t
    .model("LoudPerson", {
      name: t.string,
    })
    .actions((self) => ({
      sayName() {
        console.log(self.name.toUpperCase());
      },
    }));

  const QuietPerson = t
    .model("QuietPerson", {
      name: t.string,
    })
    .actions((self) => ({
      sayName() {
        console.log(self.name.toLowerCase());
      },
    }));

  const RegularPerson = t.compose("RegularPerson", LoudPerson, QuietPerson);

  const person = RegularPerson.create({ name: "Tyler" });
  person.sayName();
})();

This works right now: https://codesandbox.io/p/sandbox/duplicate-checks-s68vdh?file=%2Fsrc%2Findex.ts%3A5%2C23.

We may want to consider other such cases where inheritance or composition expects us to be able to re-declare some kind of view, action, or volatile state

[thegedge]

Yeah, that should be totes doable.

I guess the one case that could be problematic if X has a property with name something and Y has a view with the name something and you compose the two together. That seems kind of reasonable to do, but would 💥. I think it would 💥 even before my recent changes, so if it's something we want to support, we may have some work to do!