-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
deps: update to typescript 4.1.2 #11690
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.
LGTM but a few comment requests :)
@@ -76,7 +76,6 @@ function getHTMLImages(allElements) { | |||
cssComputedPosition: getPosition(element, computedStyle), | |||
isCss: false, | |||
isPicture, | |||
// @ts-expect-error: loading attribute not yet added to HTMLImageElement definition. |
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.
🎉
@@ -70,6 +70,7 @@ function processForProto(lhr) { | |||
|
|||
// Drop the i18n icuMessagePaths. Painful in proto, and low priority to expose currently. | |||
if (reportJson.i18n && reportJson.i18n.icuMessagePaths) { | |||
// @ts-expect-error |
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? should we set it to undefined or do we need to update the type to allow it to be undefined to reflect reality?
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.
done.
And now I realize the #10148 wouldn't work for PSI LHRs until we fix this :(
@@ -143,11 +143,12 @@ class Simulator { | |||
|
|||
/** | |||
* @param {Node} node | |||
* @return {NodeTimingIntermediate} | |||
* @return {Required<NodeTimingIntermediate>} |
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.
yeah this can be improved, filed #11692 for myself
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.
thank you @patrickhulce!
@@ -156,8 +156,8 @@ class Fetcher { | |||
iframe.src = src; | |||
iframe.onload = iframe.onerror = () => { | |||
iframe.remove(); | |||
delete iframe.onload; | |||
delete iframe.onerror; | |||
iframe.onload = null; |
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.
come to think of it, why do we do this at all? can onload
or onerror
ever refire after it's been fired before and removed from the DOM?
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 helps me sleep at night
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.
also I'm wondering if this holds a reference to itself, so GC would never delete the element ...
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.
@@ -81,6 +81,7 @@ class Util { | |||
// into 'debugdata' (LHR ≥5.0). | |||
// @ts-expect-error tsc rightly flags that these values shouldn't occur. | |||
if (audit.details.type === undefined || audit.details.type === 'diagnostic') { | |||
// @ts-expect-error |
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 makes less sense to me than the one immediately above it, what's the reason?
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 makes less sense to me than the one immediately above it, what's the reason?
it's because the conditional above only lets in audit.details
that shouldn't be possible, so it's of type never
and can't be assigned to. Definitely worth a @ts-expect-error
comment for that, though.
https://devblogs.microsoft.com/typescript/announcing-typescript-4-1/#breaking-changes
https://devblogs.microsoft.com/typescript/announcing-typescript-4-0/#breaking-changes
A fix in the JSDoc parser resulted in a handful of places where
undefined
is now properly being checked.