-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Update LKG to enable improved narrowing in 4.4. #44842
Conversation
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. |
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.