-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Don't re-alias top-level type aliases with local type aliases #43701
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
Conversation
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.
This code appears to match the explanation you gave at the design meeting, but I don't understand the feature well enough to predict possible consequences. Is there anything specific you wanted me to test/check?
@@ -50,7 +50,7 @@ tests/cases/conformance/types/conditional/conditionalTypes1.ts(159,5): error TS2 | |||
tests/cases/conformance/types/conditional/conditionalTypes1.ts(160,5): error TS2322: Type 'T' is not assignable to type 'ZeroOf<T>'. | |||
Type 'string | number' is not assignable to type 'ZeroOf<T>'. | |||
Type 'string' is not assignable to type 'ZeroOf<T>'. | |||
tests/cases/conformance/types/conditional/conditionalTypes1.ts(263,9): error TS2403: Subsequent variable declarations must have the same type. Variable 'z' must be of type 'T1', but here has type 'T2'. | |||
tests/cases/conformance/types/conditional/conditionalTypes1.ts(263,9): error TS2403: Subsequent variable declarations must have the same type. Variable 'z' must be of type 'T1', but here has type 'Foo<T & U>'. |
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.
This seems worse, but I believe I understood you to say it's just a return to our old behavior?
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.
Yes, it's a return to the old behavior, the rationale being that Foo<xxx>
is accessible outside the function whereas T2
is not. I think very little code will be affected by this change, as it is relatively rare to have local type aliases.
Not really. I suppose the debatable part is whether we just want to point users at the workaround (i.e. changing |
Is there some way we could supplement tracing to make it clear when a bad expansion happens so that we can at least point users to the relevant type declarations? If we can't assist users in finding where to apply that workaround, I'd be pretty reluctant to recommend they do so. |
@typescript-bot perf test this faster |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at e8f86b3. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at e8f86b3. You can monitor the build here. Update: The results are in! |
@DanielRosenwasser Here they are:Comparison Report - master..43701
System
Hosts
Scenarios
Developer Information: |
@DanielRosenwasser Here they are:Comparison Report - master..43701
System
Hosts
Scenarios
Developer Information: |
I can't think of any. If we preserved all possible type aliases then perhaps, but in that case we'd probably just add logic to pick the first accessible alias. But as we discussed in the design meeting, I don't think the additional complexity is merited. Overall, I think we're better off with the behavior in this PR so I'll merge it. |
TS 4.3 avoids re-aliasing type aliases needlessly, in microsoft/TypeScript#43701. This changes the way that a couple of tests in redux-orm print their type aliases.
TS 4.3 avoids re-aliasing type aliases needlessly, in microsoft/TypeScript#43701. This changes the way that a couple of tests in redux-orm print their type aliases.
Fixes #43622.