mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.9k stars 640 forks source link

Broken model types in 6.0.0 version #2186

Open dfilatov opened 5 days ago

dfilatov commented 5 days ago

Bug report

Sandbox link or minimal reproduction code

const BaseModel = types.model();
type BaseModelType = typeof BaseModel;

const DerivedModel = BaseModel.props({ a: '' });

const test: BaseModelType = DerivedModel;

Describe the expected behavior

No type errors

Describe the observed behavior

Type 'IModelType<{ a: IType<string | undefined, string, string>; }, {}, _NotCustomized, _NotCustomized>' is not assignable to type 'IModelType<{}, {}, _NotCustomized, _NotCustomized>'.
  The types of 'views(...).CreationType' are incompatible between these types.
    Type 'ModelCreationType<ExtractCFromProps<{ a: IType<string | undefined, string, string>; }>>' has no properties in common with type 'IStateTreeNode<IAnyType>'.ts(2322)

It has been broken in 6.0.0 version. It works in 5.4.1 version as expected.

coolsoftwaretyler commented 5 days ago

Thanks for the bug report @dfilatov! Sorry for the inconvenience. I will look into this soon.

coolsoftwaretyler commented 5 days ago

Hi @dfilatov - what TypeScript version are you using? I'm having trouble reproducing. Here's a CodeSandbox without errors: https://codesandbox.io/p/sandbox/issue-2186-reproduction-dgy9g3

Version 6.0.0 upgraded TypeScript to 5.3.3 which seems a possible culprit if you're seeing issues locally.

dfilatov commented 5 days ago

Hi @dfilatov - what TypeScript version are you using?

5.4.5

dfilatov commented 5 days ago

coolsoftwaretyler I've opened your link and see error:

image
coolsoftwaretyler commented 5 days ago

Oh nice. Must have been a bug on CodeSandbox.

Will continue investigating!

coolsoftwaretyler commented 3 days ago

I ran git bisect on the reproduction code you provided and it identified 46334b6d37a9fa9b9b984a07a0689391381909a3 as the commit that introduced the error.

I wonder if WithAdditionalProperties here: https://github.com/mobxjs/mobx-state-tree/commit/46334b6d37a9fa9b9b984a07a0689391381909a3#diff-8ce65433bb1651566fa2253bfc8167953b5871ef77ed073d1d47d0e4876fabdcR217 is the culprit?

Will keep investigating!

coolsoftwaretyler commented 3 days ago

Ok, looks like the issue is with ModelCreationType2 and its changes. Here is a commit that fixes your reported issue: https://github.com/mobxjs/mobx-state-tree/pull/2187/commits/86c8cf374a6f0a01425df100987597db1095e09f#diff-8ce65433bb1651566fa2253bfc8167953b5871ef77ed073d1d47d0e4876fabdcR139 (it's a little big because I'm on a new machine that doesn't seem to respect our formatting right now, my bad), but it causes other type specifications in the test suite to fail.

@dfilatov - are you only encountering this issue specifically when you create a base model with no properties? I want to figure out if this is happening because empty models are a special case or if there's a bigger problem with the props builder on models.

Let me know!

dfilatov commented 2 days ago

@dfilatov - are you only encountering this issue specifically when you create a base model with no properties?

yep, looks like it's happened with base models without own props (but with own actions/views)

coolsoftwaretyler commented 2 days ago

Very helpful! I think we just need to figure out some way to carve out the types to accommodate a model like that.

As a stopgap while we figure it out, I might suggest adding some kind of placeholder property in that model definition to clear the error if you don't want to ts-ignore/ts-expect-error