Unintended behavior when creating new models with falsy first argument

[coolsoftwaretyler]

For folks using TypeScript, they'll get a type error if they try to:

const Model = types.Model(null, { prop1: "p1"}) // Redline on the first argument.

But for JavaScript consumer, this same code won't give them any TS hints, nor will it error at runtime. But the properties will end up empty.

I think this is a reasonable failure mode - nice that it won't actually toss an exception, but maybe confusing, since we don't even log anything in dev mode.

_Originally posted by @coolsoftwaretyler in https://github.com/mobxjs/mobx-state-tree/pull/2044#discussion_r1265606927_

See thread for additional details.

To resolve this issue we should:

1. Write some new (failing) tests that describe an intended behavior (perhaps throwing an error in dev mode)
2. Change the types.model function, or ModelType constructor to handle input a little differently to pass those tests.
3. The tests added in the original PR will likely have one failure, which can be removed as well.

[coolsoftwaretyler]

If anyone is interested in grabbing this, I would be happy to discuss different approaches, help you get set up in the repo, or whatever else would make this feel accessible for first time contributors.

[nithinssabu]

I am using Mobx daily at work, but not MST. I am an experienced developer but have not used Typescript yet.

I am interested in picking this up if my experience level mentioned above is not a blocker.

[coolsoftwaretyler]

Hey @nithinssabu - thanks for your interest! That's great!

I'm gonna go ahead and assign this issue to you for now.

To begin, do you want to read through the issue, take a look at the original PR, and comment back with your understanding of the problem/how you want to approach it?

From there, we can get you started on a PR.

If you're feeling particularly industrious, I'd also be happy to start with a PR that has some test cases to demo your intended solution.

Let me know any way I can be helpful. I'm gonna be a little busy until Friday, but will try to be available for set up questions.

[nithinssabu]

I'm gonna go ahead and assign this issue to you for now.

Thank you for assigning me. I'm so excited about this.

So, here's my understanding of the problem:

Currently when we provide an invalid name value, the model will be created with the name AnonymousModel and the properties will be empty (even if provided). This can cause confusing behaviour, especially to non-TS users.

We want to throw an exception in this case instead of creating a valid model.

I would approach it this way:

• Modify this test case to assert that an exception is thrown
• Fix the behaviour to match this assertion

[coolsoftwaretyler]

Hey @nithinssabu - just about right, although after thinking about this a bit more, I think we should make this change backwards compatible by:

1. Keeping the existing test case intact and passing - for production specific environments
2. Adding a new test that throws an exception only when not in production. Here is an example where we test that specific environmental behavior.

If we do that, we should be able to include this in the next minor release, rather than waiting for a breaking change. I think very strictly speaking, this will break MST in some development builds, but keeping the new exception out of production will support MST v5 overall.

[nithinssabu]

Hi @coolsoftwaretyler Please check the linked PR and see if it meets the requirements. Also, please suggest a better error message if the one I used is not good enough.

[coolsoftwaretyler]

Thanks, @nithinssabu! I will take a look today or this weekend. Appreciate your time!

[nithinssabu]

@coolsoftwaretyler Thanks for the review and merging the PR. It would be great if you can assign to me similar smaller issues that are not urgent. I would like to actively contribute.