-
-
Notifications
You must be signed in to change notification settings - Fork 535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ReadonlyDeep): lessen instantiation excessively deep errors #650
Conversation
@sindresorhus Anything I can do to help move this forward, or more info you need from me? I'm running into these 'instantiation excessively deep' errors quite a lot with |
It would be nice to have some tests that would produce "Type instantiation is excessively deep and possibly infinite" before this change. |
Hi @sindresorhus! Ok, I've done substantially more debugging here and have updated the PR accordingly. The OP now reflects the latest changes, and I've added tests for all of them. |
25bfac2
to
98903e3
Compare
@sindresorhus How does it look now (if you have a chance to look)? Thanks! |
Thanks @sindresorhus! |
By the way, does |
I was repeatedly getting "instantiation excessively deep" errors with
ReadonlyDeep
. After more debugging, I realized that the root issue stemmed from usingReadonlyDeep
on a recursive type similar totype-fest
'sJsonValue
type. With that discovery, I was able to add a few more cases toReadonlyDeep
that fixed all the errors I was seeing, and should make the type more powerful/robust for everyone. The individual changes are:Previously,
ReadonlyDeep<JsonValue>
immediately triggered an 'instantiation excessively deep' error. Now, it does not, and it returns a type that's quite close to the type you'd expect if you manually convertedJsonValue
to readonly. The produced type is not identical (which is why the test usesexpectAssignable
rather thanexpectType
), becauseReadonlyDeep
doesn't/can't preserve the true recursive structure; instead, after about four or five levels of expansion, the type permitsany
. But it is much better than before.In order to accomplish this, I had to add an explicit case for detecting arrays. This is the only change that was required to make
ReadonlyDeep<JsonValue>
not error, I believe. This change should also prevent errors for many other recursive types involving arrays. Previously, arrays hit theReadonlyObjectDeep
case, which seemed to contribute to hitting the recursion limit.Adding the array case mentioned above led the tests for tuples to fail (as they started hitting this case, rather than
ReadonlyObjectDeep
), so I had to add cases to detect tuples specifically. I tried a number of approaches, including continuing to passT
toReadonlyObjectDeep
when it looked likeT
was a tuple, but the approach in this PR is the only one I found that passed all the tests. This change actually improves the returned type when the input type is a tuple with a leading spread. Specifically,ReadonlyDeep<[...string[], number]>
used to givereadonly [...(string | number)[], string | number]
; now it correctly producesreadonly [...string[], number]
.