-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat: update to TypeScript 4.7 #14242
Conversation
@@ -7,10 +7,6 @@ | |||
|
|||
/// <reference no-default-lib="true"/> | |||
|
|||
interface AbortSignal extends EventTarget { |
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 is now part of lib dom.
// code_actions: Option<Vec<CodeAction>>, | ||
// source: Option<Vec<SymbolDisplayPart>>, |
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 is deprecated, so we shouldn't ever use it if we need it.
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.
LGTM, should we wait until the week of June 13th before landing this PR? It appears it might be painful to not apply it in v1.22 branch
@@ -695,7 +723,7 @@ interface AbortController { | |||
/** Returns the AbortSignal object associated with this object. */ | |||
readonly signal: AbortSignal; | |||
/** Invoking this method will set this object's AbortSignal's aborted flag and signal to any observers that the associated activity is to be aborted. */ | |||
abort(reason?: any): void; | |||
// abort(): AbortSignal; - To be re-added in the future |
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.
Is this on purpose? Looks strange
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.
Agreed, this seems a bit problematic.
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 is what is upstream: https://github.com/microsoft/TypeScript/blob/release-4.7/lib/lib.webworker.d.ts#L726
I really don't want to get into updating their distributables if we can avoid it. It appears they have fixed it in main, but that is what they issued with 4.7. The impact on Deno users will be minimal, because it only gets used when users explicitly use it.
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.
LGTM too
I would rather get it in so canary has it so it isn't some big surprise when it makes the release as it may highlight more "breakages" in It is mostly atomic except for the LSP changes, and even then, it is stuff we likely won't be touching anytime soon. Also, I am away the week of June 13th. |
Okay, let's land it now then. I'll cut the 1.23.2 release and deal with any troubles then. |
No description provided.