Skip to content

Conversation

@brendankenny
Copy link
Contributor

I was doing an update to 4.3.5 when the 4.4 release candidate came out. The 4.4 perf improvements really help with the continuing split up of tsconfigs, so it would be nice to land now.

The fixes needed in existing code are kind of random but are each a reasonable change.

@brendankenny brendankenny requested a review from a team as a code owner August 12, 2021 23:08
@brendankenny brendankenny requested review from adamraine and removed request for a team August 12, 2021 23:08
@google-cla google-cla bot added the cla: yes label Aug 12, 2021
*/
const mergeOptionsOfItems = function(items) {
/** @type {Array<{id: string, path?: string, options?: Object<string, any>}>} */
/** @type {T[]} */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this was implicitly downcasting mergedItems back to T[] on return. Something in 4.3 no longer allows this, so it needs to be T[] from the start

/** @type {{
tapTargetElement: Element,
clientRects: ClientRect[]
clientRects: LH.Artifacts.Rect[]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these haven't been real ClientRects in a long time. Not sure why this wasn't previously an error.

/**
* Collapses a jsdoc comment into a single line and trims whitespace.
* @param {string=} comment
* @param {import('typescript').JSDoc['comment']} comment
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tsc 4.3 added support for @link tags in 4.3, so jsdoc comments aren't always just simple strings anymore, sometimes they're structured data. Our UIStrings jsdoc comments are still just strings for now, though, so everything still works the same.

There is some movement in their code to take a more array-of-text-nodes generalized approach to jsdoc comments, so I left a comment in here in case it ever does start failing.

// Allow users to view the runnerResult
if ('lhr' in json) {
json = /** @type {LH.RunnerResult} */ (json).lhr;
const runnerResult = /** @type {LH.RunnerResult} */ (/** @type {unknown} */ (json));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfectly reasonable complaint that the existing casts are garbage. This maintains the garbage but makes it clear we know what it is :)

}
}
} catch (/** @type {Error} */ e) {
} catch (e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only unknown is allowed for catch clause variables now

"checkJs": true,
"strict": true,
// TODO: turn this off to be fully `strict`.
"useUnknownInCatchVariables": false,
Copy link
Contributor Author

@brendankenny brendankenny Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would make all catch clause variables unknown (instead of the current default any), so we would always have to do typeof/instanceof/x in err checks before taking properties off of errors.

That's the right way to do it in JS, where code can throw whatever it wants, but we probably aren't ready to commit to that yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants