-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fix react-redux break on DT #30375
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 react-redux break on DT #30375
Conversation
@typescript-bot run dt |
Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at d0d10b8. You can monitor the build here. It should now contribute to this PR's status checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable change. A question about the baseline changes.
// Repro from #25291 | ||
|
||
type PromisedTuple<L extends any[], U = (...args: L) => void> = | ||
U extends (h: infer H, ...args: infer R) => [Promise<H>, ...PromisedTuple<R>] ? [] : [] | ||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that all these circular reference errors go away because the types are now deferred? Does this example end up with no error because the rest of the program never uses PromisedTuple, or because the error was not necessary in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It goes away because the type is deferred now, yes. getReturnTypeOfSignature
needs to be recurred into to trigger the error - but now, we ask for the return type, get the conditional, attempt to simplify the conditional, see the conditional instantiate itself into itself as part of one of its branch types, then go "nope, definitely won't simplify" and give it its deferred type, which is then what we return. Since we never actually inspect the resolved type of that inner instantiation (since we went "ah, it's going to be the same as one that's already resolving"), we never assigned it the circularity error.
The DT run is clean - the only breaks are the ramda changes from two days ago that #30363 should hopefully fix. |
This is my attempt for a fix that doesn't revert #30334. Rather than identifying where we're triggering a new instantiation, I've simply made conditional types more gracefully handle circular instantiations (which tbh I am surprised we didn't have a guard for already).
Fixes #30372