core: better type checking of evaluate() code #12795

brendankenny · 2021-07-15T23:30:42Z

explicitly require individual functions that are going to be deps for an evaluate() call. This continues the blurred line between Node and evaluate() code that runs in the browser, but a) they're already intermixed and b) this allows the results to be type checked instead of always any via // @ts-expect-error.

get rid of *String properties on pageFunctions, just use the function directly
downside is some terser minification options have to be disabled, resulting in 20% larger bundle (10% after brotli). This doesn't seem like a big deal to me (we used to only remove indentation whitespace not long ago), but there are more options I'll mention below if we'd rather take more fragile approaches
includes a hackish toString approach for getNodeDetails() so we can keep using it as it has been. See below for discussion

brendankenny · 2021-07-15T23:38:33Z

build/build-bundle.js

+ // Class and function names are needed for things like loading gatherers by
+ // name and accessing functions by name in Runtime.evaluate. Instead, just
+ // rely on whitespace elimination for the large savings.
+ compress: false,
+ mangle: false,


there are a few permutations that work here, but I'm not sure if it matters (these just affect identifiers, so more bytes but parsers blaze through them so not a big deal if you aren't downloading our bundle).

The biggest savings would be something like:

Suggested change

// Class and function names are needed for things like loading gatherers by

// name and accessing functions by name in Runtime.evaluate. Instead, just

// rely on whitespace elimination for the large savings.

compress: false,

mangle: false,

// The config relies on class names for gatherers.

keep_classnames: true,

// Runtime.evaluate errors if function names are elided.

keep_fnames: true,

mangle: {

reserved: Object.keys(pageFunctions),

},

which is essentially no change in size over the current bundle but would be more fragile (either the function would need to be local to the file or in page-functions). No problem today but if you did import someone else's function it wouldn't necessarily be obvious why it was broken.

brendankenny · 2021-07-15T23:45:46Z

lighthouse-core/lib/page-functions.js

+// HACK: `getNodeDetails` is a function used in many places with multiple deps
+// that would need to be listed in the `evaluate()` call every time it's used.
+// The deps could be inlined, but they need to be available externally for
+// testing. So, override `getNodeDetails.toString()` to include all the deps.


what this says. All of the functions used in here are only used inside getNodeDetails except getBoundingClientRect (which would be easy to replace in full-page-screenshot if need be) so they could all be defined internally to a getNodeDetails wrapping function but...it is very convenient to test them all separately. Redefining toString seems ok to me here, but we could rework if it's too ugly for anyone.

this is hilarious

brendankenny · 2021-07-15T23:59:11Z

Oh yeah, and the relevance to #12771: page functions won't be treated as globals by bundlers, so no need to do any patching of the bundled output.

connorjclark · 2021-07-16T00:26:12Z

lighthouse-core/gather/gatherers/anchor-elements.js

 const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
 const dom = require('../driver/dom.js');
-const pageFunctions = require('../../lib/page-functions.js');
+const {getElementsInDocument, getNodeDetails} = require('../../lib/page-functions.js');


(you probably already know this, just wanted to make it explicit)

FWIW this is what breaks terser. this is basically

const getElementsInDocument = require('../../lib/page-functions.js').getElementsInDocument;

which allows terser to do w/e it wants with the name here since it is just local.

brendankenny · 2021-07-19T22:13:43Z

went with the mangle: {reserved: Object.keys(pageFunctions)} line, so bundle size change is miniscule. Will have to double check with #12771 that this works with rollup and not just browserify.

brendankenny · 2021-07-19T22:16:23Z

hmm, somehow this makes the devtools bundle 41KiB smaller even though I would have thought reserving the names would make it bigger (since there's a few names we won't have minified that would have been minified before).

Maybe there's a default set of reserved names this is replacing? I ran the bundle tests locally but I guess lets see how the DT tests go...

connorjclark

left some nits in comments

but there are more options I'll mention below if we'd rather take more fragile approaches

Besides injecting pageFunctions consisting of w/e passed to deps, what options are there?

if we're able to keep the bundle size to ~the same with the custom mangle settings then I'm all for the approach in the PR. otherwise, I'll begrudgingly still accept it and its 20% addition to bundle size :)

double check with #12771

I'll check this.

Maybe there's a default set of reserved names this is replacing? I ran the bundle tests locally but I guess lets see how the DT tests go

seeing this in GHA bundled smoke test for tap targets:

✘ difference at TapTargets artifact[0]
              expected: {"node":{"snippet":"/large-link-at-bottom-of-page/"}}
                 found: undefined

          found result:
      {
        "__failedInBrowser": true,
        "name": "ReferenceError",
        "message": "a is not defined"
      }

can you update the PR with master to pick up the recent CDT test fixes?

connorjclark · 2021-07-21T19:47:20Z

lighthouse-core/gather/gatherers/meta-elements.js


 /* c8 ignore start */
 function collectMetaElements() {
- const functions = /** @type {typeof pageFunctions} */({


so long sweet summer child

connorjclark · 2021-07-21T19:48:57Z

lighthouse-core/lib/page-functions.js

@@ -13,10 +13,11 @@
 * the c8 code coverage tool. See c8.sh
 *
 * Important: this module should only be imported like this:
- * const pageFunctions = require('...');


missing the "do it this way" example now.

could keep the same comment and just swap the examples?

connorjclark · 2021-07-21T19:49:58Z

lighthouse-core/lib/page-functions.js

+// that would need to be listed in the `evaluate()` call every time it's used.
+// The deps could be inlined, but they need to be available externally for
+// testing. So, override `getNodeDetails.toString()` to include all the deps.
+/** @type {string} */


Suggested change

/** @type {string} */

seems obvious what the type of toString() is :)

connorjclark · 2021-07-21T19:50:44Z

lighthouse-core/lib/page-functions.js

+ ${getBoundingClientRect.toString()}
+ ${getOuterHTMLSnippet.toString()}
+ ${getNodeLabel.toString()}
+ ${originalgetNodeDetailsToString}`;


originalGetNodeDetailsToString ^

connorjclark · 2021-07-21T19:54:14Z

lighthouse-core/gather/gatherers/trace-elements.js

@@ -294,7 +293,7 @@ class TraceElements extends FRGatherer {
 objectId,


I thought we had a version of this on driver/executeContext that was typed like execute and also handled the boilerplate here... but I guess we don't. it's the only instance of Runtime.callFunctionOn so it makes sense.

connorjclark · 2021-07-21T20:12:28Z

lighthouse-core/gather/driver/execution-context.js

@@ -94,7 +94,7 @@ class ExecutionContext {
 return new Promise(function (resolve) {
 return Promise.resolve()
 .then(_ => ${expression})
- .catch(${pageFunctions.wrapRuntimeEvalErrorInBrowserString})
+ .catch(${pageFunctions.wrapRuntimeEvalErrorInBrowser.toString()})


why call toString explicitly when all objects in a template string are coerced via .toString() automatically?

connorjclark · 2021-07-21T20:45:20Z

pushed to esmodules-rollup-try-mangle. 29dd996

removing the postprocess stuff and just using mangle didn't work ... rollup has already renamed those functions before terser ever gets a chance to do anything. With terser disabled, image-elements looks like this:

// source
const {
  getElementsInDocument,
  getBoundingClientRect,
  getNodeDetails,
} = require('../../lib/page-functions.js');

// generated
const {
	  getElementsInDocument: getElementsInDocument$5,
	  getBoundingClientRect,
	  getNodeDetails: getNodeDetails$6,
	} = pageFunctions;

// ....

function collectImageElementInfo() {
	  const allElements = getElementsInDocument$5('*'); // does not exist in runtime.evaluate
	  return getHTMLImages(allElements).concat(getCSSImages(allElements));
	}

And pretty much every audit errored with a similar reason. can repro in that branch with yarn open-devtools (may need to delete your devtools .tmp cache)

brendankenny · 2021-07-23T22:29:47Z

removing the postprocess stuff and just using mangle didn't work ... rollup has already renamed those functions before terser ever gets a chance to do anything. With terser disabled, image-elements looks like this:

// generated
const {
  getElementsInDocument: getElementsInDocument$5,
  getBoundingClientRect,
  getNodeDetails: getNodeDetails$6,
} = pageFunctions;

weird, can you tell why rollup still thinks it's a global?

seeing this in GHA bundled smoke test for tap targets:
"message": "a is not defined"

yeah, hmm...

brendankenny · 2021-07-23T22:34:34Z

seeing this in GHA bundled smoke test for tap targets:
"message": "a is not defined"

yeah, hmm...

oh right, because tap-targets also imports rect-helpers.js

core: better type checking of evaluate() code

00272f1

brendankenny requested a review from a team as a code owner July 15, 2021 23:30

brendankenny requested review from connorjclark and removed request for a team July 15, 2021 23:30

google-cla bot added the cla: yes label Jul 15, 2021

brendankenny mentioned this pull request Jul 15, 2021

misc(build): use rollup to build lighthouse-core bundles #12771

Merged

devtools-bot assigned connorjclark Jul 15, 2021

devtools-bot added the waiting4reviewer label Jul 15, 2021

brendankenny commented Jul 15, 2021

View reviewed changes

connorjclark reviewed Jul 16, 2021

View reviewed changes

don't mangle page-function names

16cee4b

vercel bot deployed to Preview July 19, 2021 22:06 View deployment

connorjclark reviewed Jul 21, 2021

View reviewed changes

connorjclark added waiting4committer and removed waiting4reviewer labels Jul 23, 2021

connorjclark mentioned this pull request Apr 25, 2022

Investigate switching to esbuild #13906

Closed

connorjclark closed this Apr 25, 2022

connorjclark mentioned this pull request Jul 11, 2022

core: convert to ES modules #14182

Merged

connorjclark mentioned this pull request Jul 18, 2023

tests: add page functions bundling test #15280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: better type checking of evaluate() code #12795

core: better type checking of evaluate() code #12795

brendankenny commented Jul 15, 2021

brendankenny Jul 15, 2021 •

edited

Loading

brendankenny Jul 15, 2021

connorjclark Jul 16, 2021

brendankenny commented Jul 15, 2021

connorjclark Jul 16, 2021

brendankenny commented Jul 19, 2021

brendankenny commented Jul 19, 2021

connorjclark left a comment

connorjclark Jul 21, 2021

connorjclark Jul 21, 2021

connorjclark Jul 21, 2021

connorjclark Jul 21, 2021

connorjclark Jul 21, 2021

connorjclark Jul 21, 2021

connorjclark commented Jul 21, 2021

brendankenny commented Jul 23, 2021

brendankenny commented Jul 23, 2021

- // Class and function names are needed for things like loading gatherers by
- // name and accessing functions by name in Runtime.evaluate. Instead, just
- // rely on whitespace elimination for the large savings.
- compress: false,
- mangle: false,
+ // The config relies on class names for gatherers.
+ keep_classnames: true,
+ // Runtime.evaluate errors if function names are elided.
+ keep_fnames: true,
+ mangle: {
+ reserved: Object.keys(pageFunctions),
+ },

		@@ -294,7 +293,7 @@ class TraceElements extends FRGatherer {
		objectId,

core: better type checking of evaluate() code #12795

core: better type checking of evaluate() code #12795

Conversation

brendankenny commented Jul 15, 2021

brendankenny Jul 15, 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 commented Jul 15, 2021

Choose a reason for hiding this comment

brendankenny commented Jul 19, 2021

brendankenny commented Jul 19, 2021

connorjclark left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Jul 21, 2021

brendankenny commented Jul 23, 2021

brendankenny commented Jul 23, 2021

brendankenny Jul 15, 2021 •

edited

Loading