Ensure actions/volatile state also throw when overriding props.

[thegedge]

What does this PR do and why?

If we allow instantiation to proceed further, the instantiation will fail, but something about the snapshot reaction we set up leaks and causes future tests to fail. This is a follow-up to #2204 where we made a similar change for views.

Steps to validate locally

Revert the changes to model.ts and run bun test to observe various tests failing due to this "leak".

[thegedge]

Worth pointing out my attempt to tackle the leak without changing any behaviour:

master...thegedge:make-bun-test-work-other#diff-64a8b704da

It sort of worked, but I couldn't 100% it. Regardless, I like the current solution since we avoid wasting time setting up the snapshot reaction that is guaranteed to fail. @coolsoftwaretyler also set up a discussion where we could consider moving this even earlier, when building models: #2206

[coolsoftwaretyler]

Excellent! Will take a look this weekend!

[coolsoftwaretyler]

Hey @thegedge - this is great and makes a lot of sense based on our prior discussion.

My inclination is to merge this as-is and bring it along for the breaking v7. But, as I was reading the changes, I found myself asking "Can we make these errors more specific, and let users know where the duplicate keys are? Can we tell them if it's a duplicated property, view, or even action?"

Then I was like "wait, do we even check for that?" And no, we do not. See this CodeSandbox: https://codesandbox.io/p/sandbox/duplicate-views-no-error-92z9c7?file=%2Fsrc%2Findex.ts%3A18%2C24

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'
})();

Technically that duplicate something view will get a redline, but the duplicated anAction action does not trigger any sort of author-time errors, and none of those things trigger runtime errors.

This is not something MST has ever promised, as far as I know, and in many ways it does behave intuitively (the last declaration wins). But while we're looking at this error handling, I thought it might be a good moment to ask ourselves if this is useful in userland or not.

I don't feel as though we need to block this PR to implement this, but if you think there's merit to the idea, this is also a good point for us to look at it.

What do you think?

[thegedge]

But while we're looking at this error handling, I thought it might be a good moment to ask ourselves if this is useful in userland or not.

What do you think?

My two cents would be that, if not always, it would almost always a mistake. I'd be down for following up with something like, or if you'd like, happy to add it to this PR!

[thegedge]

Oh yeah, one other thought. If ☝️ is correct in that this is always / almost always a mistake, I prefer leaning towards throwing by default, and having facilities to bypass our safeties when necessary. I'd love to see that use case though, as I can't think of one :)

[coolsoftwaretyler]

Since this PR is well-contained and already a follow-up itself, let's merge this and I'll draft up a new issue for the duplicate detection so we can add that incrementally.

I'll tag you there and you can pick it up if it interests you, but since we've never ensured that de-duplication, I have very much no urgency around it.