-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Update LKG to enable improved narrowing in 4.4. #44842
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
| ); | ||
| const expr = some(pendingExpressions) ? | ||
| factory.inlineExpressions(compact([...pendingExpressions!, node])) : | ||
| factory.inlineExpressions(compact([...pendingExpressions, node])) : |
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 one is actually unfortunate - pendingExpressions is actually typed as never because we have an earlier assignment of undefined followed by a check for some(...) which is a type predicate on readonly T[]. Since you can't narrow down a readonly T[] from an undefined, it leaves us with never.
The real culprit is #9998, but it is also strange that we just allow a spread of type never.
|
I did notice one opportunity for a new narrowing: accessing a property on a non-null value. When you write if (foo!.bar) {
// ...
}You shouldn't have to repeat the if (foo!.bar) {
foo.bar.baz()
}I think the only downside is that it's sometimes nice to ensure all uses of a variable look similar. |
sandersn
left a comment
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.
How did you find all the places to remove !? Did the linter point them out for you?
|
At first it was manual just to get a sense of some patterns it would/wouldn't catch, but then yes, I ran |
| "hashchange": HashChangeEvent; | ||
| "gamepadconnected": Event; | ||
| "gamepaddisconnected": Event; | ||
| "hashchange": Event; |
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.
Why this is changed to Evnet from HashChangeEvent?
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.
These are auto-generated, so they might need to be fixed up. Maybe @saschanaz or @orta have more context here.
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 got added back: microsoft/TypeScript-DOM-lib-generator#1104
| interface SVGElementInstance extends EventTarget { | ||
| readonly correspondingElement: SVGElement; | ||
| readonly correspondingUseElement: SVGUseElement; | ||
| } |
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.
These properties used to be available on SVGElement, which used to extend SVGElementInstance in LOC 12903. They disappeared during the upgrade from v4.3.5 to v4.4.2.
Reported here: microsoft/TypeScript-DOM-lib-generator#1023 (comment)
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.
Removal was intentional, as per microsoft/TypeScript-DOM-lib-generator#1023 (comment):
Those are not available on any modern browser nor MDN, and thus are probably IE-specific properties. The removal is intended since the types library intends to only include things that exist in modern browsers.
Fixes #44841.