core: add type checking to page functions #11958

brendankenny · 2021-01-14T18:31:54Z

The file was actually in pretty good shape, and only needed three // @ts-expect-errors, two of which could be worked around instead (but which approach to take is arguable) and the other error will hopefully be fixed in future tsc updates.

This should give a lot more confidence when making changes to e.g. nodeDetails and the like in the future.

lighthouse-core/lib/page-functions.js

brendankenny · 2021-01-14T18:33:18Z

lighthouse-core/gather/driver.js

@@ -1112,7 +1112,6 @@ class Driver {
  async cacheNatives() {
    await this.evaluateScriptOnNewDocument(`
        window.__nativePromise = Promise;
-        window.__nativeError = Error;


AFAICT this hasn't been used since #5509

brendankenny · 2021-01-14T18:35:25Z

lighthouse-core/lib/page-functions.js

+  if (!err || typeof err === 'string') {
+    err = new Error(err);
+  }


the research for what can end up in here was so long ago that I didn't want to break anything, so this is just a cleaned up version of what already existed. Also added tests to make sure all the cases are handled.

brendankenny · 2021-01-14T18:35:49Z

lighthouse-core/lib/page-functions.js

-  //   FIXME COMPAT: This hack isn't neccessary as of Chrome 62.0.3176.0
-  //   https://bugs.chromium.org/p/chromium/issues/detail?id=742530#c7


fix shipped to stable Chrome in 2017

brendankenny · 2021-01-14T18:37:08Z

lighthouse-core/lib/page-functions.js

+  if (element instanceof ShadowRoot) {
+    element = element.host;
+  }


the catch block below relies on element not being a ShadowRoot anymore, so this needs to be outside of the try block. From the PR that added it, it seems like the concern was the clone throwing, not this step anyways

brendankenny · 2021-01-14T18:38:09Z

lighthouse-core/lib/page-functions.js

+    /** @type {Element} */
+    // @ts-expect-error - clone will be same type as element - see https://github.com/microsoft/TypeScript/issues/283
    const clone = element.cloneNode();


clone is an Element since element is an Element, but the tsc dom types don't reflect this. When the mentioned issue (microsoft/TypeScript#283) is fixed (a fix was tried last year but backed out when it broke some stuff), this @ts-expect-error will start erroring and can be removed

brendankenny · 2021-01-14T18:39:42Z

lighthouse-core/lib/page-functions.js

@@ -288,24 +292,26 @@ function getNodePath(node) {
    while (prevNode = node.previousSibling) {
      node = prevNode;
      // skip empty text nodes
-      if (node.nodeType === Node.TEXT_NODE && node.nodeValue.trim().length === 0) continue;
+      if (node.nodeType === Node.TEXT_NODE && (node.nodeValue || '').trim().length === 0) continue;


nodeValue technically can be null*, so, fine

*(though this code shouldn't ever end up on a node where it is)

brendankenny · 2021-01-14T18:41:06Z

lighthouse-core/lib/page-functions.js

@@ -316,28 +322,28 @@ function getNodePath(node) {
 *  - nodePath: 0,HTML,1,BODY,1,DIV,a,#document-fragment,0,SECTION,0,IMG
 *  - nodeSelector: section > img
 */
-function getNodeSelector(node) {
+function getNodeSelector(element) {


we require this to be an Element (in the existing type, but also because we access classList and parentElement), so changed the param name, but it seemed like too much to rename the function to getElementSelector...

brendankenny · 2021-01-14T18:42:45Z

lighthouse-core/lib/page-functions.js

@@ -354,7 +360,7 @@ function getNodeSelector(node) {
 function isPositionFixed(element) {
  /**
   * @param {HTMLElement} element
-   * @param {string} attr
+   * @param {'overflowY'|'position'} attr


this was just easier since CSSStyleDeclaration doesn't have a string index, and CSSStyleDeclaration[keyof CSSStyleDeclaration] includes numbers and null. getStyleAttrValue is an inner convenience function, and it's unlikely to ever see any more values here (but more could always be added, of course) so this seems fine

brendankenny · 2021-01-14T18:43:47Z

lighthouse-core/lib/page-functions.js

  // html and body content is too broad to be useful, since they contain all page content
  if (tagName !== 'html' && tagName !== 'body') {
-    const nodeLabel = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');
+    const nodeLabel = element instanceof HTMLElement && element.innerText ||


innerText is only on HTMLElement (though innerHTML is on Element 🤷)

brendankenny · 2021-01-14T18:47:36Z

lighthouse-core/lib/page-functions.js

  };
  window.requestIdleCallback.toString = () => {
    return 'function requestIdleCallback() { [native code] }';
  };
 }

 /**
- * @param {HTMLElement} element
+ * @param {Element|ShadowRoot} element


made the HTMLElement -> Element change for two reasons:

if element is a ShadowRoot, element.host is an Element not an HTMLElement, so would really have needed a second test below to make sure it was still an HTMLElement

buuut, there doesn't appear to be any reason to limit getNodeDetails to HTMLElement...it seems reasonable for a gatherer to want to get nodeDetails of an SVG element, for instance

There aren't any significant effects from this change, just some type ones (like __lighthouseNodesDontTouchOrAllVarianceGoesAway becomes a Map<Element, string>)

lighthouse-core/lib/page-functions.js

patrickhulce · 2021-01-14T21:01:11Z

lighthouse-core/lib/page-functions.js

    const maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);
-    canvas = gl = undefined; // Cleanup for GC


I didn't understand why this was necessary at the time because I thought escape analysis is the same either way and the memory leak potential is in the side-effects of creating a webgl canvas creating a reference (in which case canvas = gl = undefined does nothing to help that, right?), but supposedly there was still a reason for this.

Given how fresh it is though, maybe @paulirish can clarify where #11847 (comment) ended up from his perspective :)

I think it started from this offhand comment, not a detected problem #11847 (comment)

But yeah, they're references to DOM objects. Unsetting the locals isn't going to affect their lifecycle after the function is done.

lighthouse-core/lib/page-functions.js

patrickhulce · 2021-01-14T21:42:31Z

types/externs.d.ts

    /** Used by FullPageScreenshot gatherer. */
-    __lighthouseNodesDontTouchOrAllVarianceGoesAway: Map<HTMLElement, string>;
+    __lighthouseNodesDontTouchOrAllVarianceGoesAway: Map<Element, string>;


hey wait a second how did this name end up staying like this 😆 I thought we had like 3 👎 comments

hey wait a second how did this name end up staying like this 😆 I thought we had like 3 👎 comments

ha, yes, I would have also preferred a different name there :)

lol I only recall @brendankenny downvoting. seems I snuck some personality in the code without any approval, sry.

to be clear this is referencing React's __SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED. but it's also a tounge-in-cheek response to some user expectations (the classic "I ran LH twice and got 91 and 92, which is it?!" that Patrick likes to reference)

Haha I loved the concept I just didn't understand why it was backwards!! I'm so onboard for __lighthouseNodesDontTouchOrAllVarianceGoesThroughTheRoof :)

I suppose I'm hoping someone finds it and suggests removing it to fix our variance problem..

core: add type checking to page functions

0075e53

brendankenny requested a review from a team as a code owner January 14, 2021 18:31

brendankenny requested review from patrickhulce and removed request for a team January 14, 2021 18:31

google-cla bot added the cla: yes label Jan 14, 2021

devtools-bot assigned patrickhulce Jan 14, 2021

devtools-bot added the waiting4reviewer label Jan 14, 2021

connorjclark reviewed Jan 14, 2021

View reviewed changes

lighthouse-core/lib/page-functions.js Outdated Show resolved Hide resolved

brendankenny commented Jan 14, 2021

View reviewed changes

lighthouse-core/lib/page-functions.js Outdated Show resolved Hide resolved

Update lighthouse-core/lib/page-functions.js

886eafe

vercel bot deployed to Preview January 14, 2021 19:09 View deployment

no need to stare into the void

1273356

vercel bot deployed to Preview January 14, 2021 21:20 View deployment

patrickhulce approved these changes Jan 14, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 14, 2021

brendankenny added the land-when-ci-is-green label Jan 15, 2021

devtools-bot merged commit 7829b85 into master Jan 15, 2021

devtools-bot deleted the typedpagefns branch January 15, 2021 00:33

devtools-bot removed the land-when-ci-is-green label Jan 15, 2021

paulirish pushed a commit that referenced this pull request Mar 23, 2021

core: add type checking to page functions (#11958)

3810a22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add type checking to page functions #11958

core: add type checking to page functions #11958

brendankenny commented Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021 •

edited

Loading

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021

brendankenny Jan 14, 2021 •

edited

Loading

patrickhulce Jan 14, 2021

brendankenny Jan 14, 2021 •

edited

Loading

patrickhulce Jan 14, 2021

brendankenny Jan 14, 2021

connorjclark Jan 14, 2021 •

edited

Loading

patrickhulce Jan 14, 2021

connorjclark Jan 14, 2021

		// FIXME COMPAT: This hack isn't neccessary as of Chrome 62.0.3176.0
		// https://bugs.chromium.org/p/chromium/issues/detail?id=742530#c7

		const maxTextureSize = gl.getParameter(gl.MAX_TEXTURE_SIZE);
		canvas = gl = undefined; // Cleanup for GC

core: add type checking to page functions #11958

core: add type checking to page functions #11958

Conversation

brendankenny commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 14, 2021 •

edited

Loading

brendankenny Jan 14, 2021 •

edited

Loading

brendankenny Jan 14, 2021 •

edited

Loading

connorjclark Jan 14, 2021 •

edited

Loading