v7.0.0. type instantiation is excessively deep and possibly infinite

[AaronPorts]

I'm using nested store structure:

const ModelProps = Model.props(aProp).props(bProp).props(cProps);
export interface IModelProps extends Instance<typeof ModelProps> {}

const ModelVolatile = ModelProps.volatile(aVolatiles).volatile(bVolatiles).volatile(cVolatiles);
export interface IModelVolatile extends Instance<typeof ModelVolatile> {}

const ModelViews = ModelVolatile.views(aViews);
export interface IModelViews extends Instance<typeof ModelViews> {}

const AAction = ModelViews.actions(a1Actions).actions(a2Actions);
export interface IAAction extends Instance<typeof AAction> {}
const BAction = AAction.actions(b1Actions).actions(b2Actions);
export interface IBAction extends Instance<typeof BAction> {}
...

After 7.0.0 update I'm getting 2589 typescript error: type instantiation is excessively deep and possibly infinite.

[coolsoftwaretyler]

Hey @AaronPorts - sorry for the inconvenience!

Seems likely related to #2199.

Any chance you can provide a minimal reproduction alongside the code snippet? The problem makes sense to me, but before we work on a resolution, I'd like to start with a test case that reproduces.

[AaronPorts]

Sure. https://github.com/AaronPorts/mobx-state-tree-test

[coolsoftwaretyler]

Thanks! I'll take a look!

[coolsoftwaretyler]

Initial investigation seems to point to the volatile builder methods as the culprit. I tried wrapping up all the props in one .props builder, and nothing changed.

But when I removed one .volatile call, a few type errors went away on IAction1. When I wrapped up all of the volatile calls into one volatile block, all type errors went away, and type resolution definitely got noticeably faster.

I wonder if we made something about volatile type inference too slow for TypeScript's liking. We made some changes in to Volatile in #2207, but I may just work on a git bisect to find out where this breaks.

[AaronPorts]

Seems OmitMerge type creates too much overhead

[coolsoftwaretyler]

OmitMerge was introduced in #2218.

aeb4e8a was the commit right before that. And when I put in your reproducer to the test suite, the TS errors go away on that commit.

They come back on 96f2e46, so I think you're correct that OmitMerge is the culprit.

[coolsoftwaretyler]

I think we got a fix in #2231. I'll try and get a review and ship it out. Thanks for a great issue reproduction and all the help here, @AaronPorts.

[coolsoftwaretyler]

Hey @AaronPorts - just merged. You should be able to grab that patch and fix with patch-package locally. I'll try and run a patch build sometime soon for 7.0.1

[AaronPorts]

It seems the problem is still occurring when I use chained actions. I have updated my repo to reproduce the issue.

[coolsoftwaretyler]

Thanks for the follow up @AaronPorts. I'll reopen and investigate.

[coolsoftwaretyler]

Confirmed! I'll see what I can do here.

[coolsoftwaretyler]

@thegedge - I've been trying to see if I can fix this with other ways to write OmitMerge, but nothing seems to be fast enough other than a simple A & B.

I think we may have to revert #2218. The options as I see it are:

1. Leave Fix model types when extending in any way. #2218 so that the types more accurately model the runtime behavior. This limits users from being able to write as many extension calls as they want.
2. Revert Fix model types when extending in any way. #2218 so that users can write as many extension calls as they want, but sometimes the types will be stricter than they ought to be

I think reverting makes more sense because:

1. My intuition says people more often want to extend models deeply than they want to overwrite prop/view/action names but change the required level of those names.
2. Even if a user does clobber with an optional value, it's possible that keeping the required value is solvable in userland with a TS directive, or just providing a default value anyway. On the flip side: this excessively deep type instantiation doesn't have a good userland workaround that I can tell - we're just blowing up the TS stack at some point with the types.

If we get around to rewriting the type systems, we can use the test cases provided by @AaronPorts as a good check to see if we have a sufficiently performant set of types to support a lot more extensions. We can also prioritize fixing the typing for chained extensions so that people don't need to do this as much as they are.

I'm going to hold off on a change here for a few days in case you've got a better idea. But if you don't have the time to think about it too much, reverting is an easy path for now and should resolve these issues.

[AaronPorts]

I guess revert changes is better option until either types rewrite or microsoft/TypeScript#34933 fix.

[thegedge]

I'll poke around, but I'm doubtful we'll be able to make both work with TS as it stands today. If I had to guess, this is likely a matter of too many instantiations (i.e., not depth) and it'll be hard for us to fix that.

Unfortunate that we'd have to make our types incorrect in the override scenario, but agreed with your intuition on this issue being worse than that scenario, @coolsoftwaretyler.

[EDIT]
Yeah, I couldn't find any way to make this work. I think the complexity of the MST types + builder pattern just doesn't play well. I think TS is unlikely to solve this for us (you never know though!) unless they give us the ability to let the typechecker do something like "compact this type and cache".

Or perhaps there's some other way to architect these types to make it all work. If anyone has ideas, throw 'em my way and I'd be happy to try it out!

[coolsoftwaretyler]

Thanks for trying, @thegedge!

I'll revert those changes, and we can revisit the root problem (the builder pattern) once we have a clearer roadmap. I should have some more info for ya in early 2026 on my end.

[coolsoftwaretyler]

Revert PR here: #2234

[coolsoftwaretyler]

Fix released in 7.0.1: https://github.com/mobxjs/mobx-state-tree/releases/tag/v7.0.1