Skip to content
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

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

ethanresnick
Copy link
Contributor

@ethanresnick ethanresnick commented Jul 23, 2023

I was repeatedly getting "instantiation excessively deep" errors with ReadonlyDeep. After more debugging, I realized that the root issue stemmed from using ReadonlyDeep on a recursive type similar to type-fest's JsonValue type. With that discovery, I was able to add a few more cases to ReadonlyDeep that fixed all the errors I was seeing, and should make the type more powerful/robust for everyone. The individual changes are:

  1. 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 converted JsonValue to readonly. The produced type is not identical (which is why the test uses expectAssignable rather than expectType), because ReadonlyDeep doesn't/can't preserve the true recursive structure; instead, after about four or five levels of expansion, the type permits any. 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 the ReadonlyObjectDeep case, which seemed to contribute to hitting the recursion limit.

  2. 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 pass T to ReadonlyObjectDeep when it looked like T 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 give readonly [...(string | number)[], string | number]; now it correctly produces readonly [...string[], number].

@ethanresnick
Copy link
Contributor Author

@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 ReadonlyDeep and would love to see this merged.

source/readonly-deep.d.ts Outdated Show resolved Hide resolved
source/readonly-deep.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

It would be nice to have some tests that would produce "Type instantiation is excessively deep and possibly infinite" before this change.

@ethanresnick
Copy link
Contributor Author

ethanresnick commented Jul 30, 2023

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.

@ethanresnick
Copy link
Contributor Author

@sindresorhus How does it look now (if you have a chance to look)? Thanks!

@sindresorhus sindresorhus merged commit cff9808 into sindresorhus:main Aug 8, 2023
5 checks passed
@ethanresnick
Copy link
Contributor Author

Thanks @sindresorhus!

@ehoogeveen-medweb
Copy link

By the way, does WritableDeep suffer from the same issues or is it unaffected? The implementations were very similar before this change.

@sindresorhus
Copy link
Owner

#664

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants