core(driver): create eval code using interface #10816

connorjclark · 2020-05-20T01:39:36Z

Idea 1 from #10781

Where simple, I also converted from getElementsInDocumentString to getElementsInDocument. Skipped for tap-targets, was too much.

Copied HTMLElementTagNameMap from dom.js for getElementsInDocument. I renamed it because TS wasn't picking it up, I'm surprised it even worked in dom.js because it's overriding the same identifier.

brendankenny

This is looking great.

It is still going to be weird/misleading to have files that are half running in node and half running in the browser so I think it's probably worth opening up to the eng sync next week, but I'm feeling net-good about the tradeoff :)

Copied HTMLElementTagNameMap from dom.js for getElementsInDocument. I renamed it because TS wasn't picking it up, I'm surprised it even worked in dom.js because it's overriding the same identifier.

Isn't it called HTMLElementByTagName though? I don't think it should be overriding anything..

brendankenny · 2020-05-20T18:36:54Z

lighthouse-core/gather/driver.js

+   * @template T, R
+   * @param {string | ((...args: T[]) => R)} expression
+   * @param {{useIsolation?: boolean, mode?: 'iffe'|'function', args?: T[], deps?: (Function|string)[]}=} options
+   * @return {Promise<R>}


Should we just split these into two eval functions?. Keep evaluateAsync and make a new evaluateFunction or something:

js-style "overloading" tends to grow in implementation complications over time and is harder to understand as a caller (e.g. options could be included with a string expression, but would it do anything in that case? Now we have to document that, etc)

There seems to be a bug in jsdoc generics where R defaults to any here if expression is a string (so there's no inferred R). In the typescript equivalent R will default to unknown (as of Typescript 3.5). If that gets fixed for JS, all the evaluateAsync(string) calls will have to be changed too. I can't think of a great way to set the return type the right way since you still can't do generic defaults or conditional types in jsdoc (AFAIK).

("fun" fact: evaluateAsync is called evaluateAsync and not evaluate because way back, "evaluate" sounded synchronous and we didn't have types to give IDE clues or type checking to ensure that calling code would handle a promised return value. That does make variations on evaluateAsync a little awkward, though, since the "Async" part seems really unnecessary now :)

Thanks for the great feedback!

js-style "overloading" tends to grow in implementation complications over time and is harder to understand as a caller (e.g. options could be included with a string expression, but would it do anything in that case? Now we have to document that, etc)

I took a second pass to incorporate this feedback, and added some docs. Running to a meeting, will respond to the rest later.

Would you prefer splitting this out to evaluate(fn, ...) and evaluateString(code, ...) (alias for evalauteAsync)?

rather than add another evaluate*() hop with _evaluate, what do you think about keeping it called evaluateAsync, so

evaluate(fn, ...) calls evaluateAsync(string, opts) calls _evaluateInContext(string, contextId)

then in the next breaking change we either incorporate the guts of evaluateAsync into evaluate so it's back to only two functions, or we rename evaluateAsync to evaluateString or whatever if we do want to keep it around for folks to use?

I see the reason for doing it the _evaluate way, but the number of evaluate layers doesn't seem worth it when external to driver we still only have evaluate() and evaluateAsync either way.

I'm a little confused about the purpose of _evaluate. Is it for a possible patch where evaluateAsync is removed but we still need a function for executing short expressions?

I think I just forgot to delete _evaluate.

connorjclark · 2020-05-20T19:56:35Z

Isn't it called HTMLElementByTagName though? I don't think it should be overriding anything..

Yeah, I messed up the copy/paste and got confused. Put the names back. Should one of these files do a type import for the other?

connorjclark · 2020-05-20T19:57:19Z

lighthouse-core/gather/driver.js

+   * @return {Promise<*>}
+   */
+  async evaluateAsync(expression, options = {}) {
+    return this.evaluate(expression, options);


Could drop this. if we like this, I'd follow up with a PR that changes all existing usages to this.

connorjclark · 2020-05-20T19:57:53Z

lighthouse-core/gather/driver.js

+   * See the documentation for `createEvalCode` in `page-functions.js`.
+   * @template T, R
+   * @param {string | ((...args: T[]) => R)} expressionOrMainFn
+   * @param {{useIsolation?: boolean, args?: T[], deps?: (Function|string)[]}=} options


FYI I would like to drop the string part eventually.

FYI I would like to drop the string part eventually.

could we drop it now and say "if you still want to use a string use the legacy evaluateAsync()"?

brendankenny · 2020-05-20T20:06:27Z

Should one of these files do a type import for the other?

I had the same thought but the files are in such different contexts I don't know which one should import from the other :)

edit: maybe it doesn't matter and it could be in both. dom.js is kind of the root for DOM stuff in the renderer so it makes sense for other renderer files to import from there, same with page-functions.js being kind of the root for driver.evaluate DOM stuff.

connorjclark · 2020-06-04T19:00:10Z

@brendankenny any more feedback?

lighthouse-core/gather/driver.js

brendankenny · 2020-06-04T21:27:22Z

lighthouse-core/gather/driver.js

+   * @template T, R
+   * @param {string | ((...args: T[]) => R)} expression
+   * @param {{useIsolation?: boolean, mode?: 'iffe'|'function', args?: T[], deps?: (Function|string)[]}=} options
+   * @return {Promise<R>}


rather than add another evaluate*() hop with _evaluate, what do you think about keeping it called evaluateAsync, so

evaluate(fn, ...) calls evaluateAsync(string, opts) calls _evaluateInContext(string, contextId)

then in the next breaking change we either incorporate the guts of evaluateAsync into evaluate so it's back to only two functions, or we rename evaluateAsync to evaluateString or whatever if we do want to keep it around for folks to use?

I see the reason for doing it the _evaluate way, but the number of evaluate layers doesn't seem worth it when external to driver we still only have evaluate() and evaluateAsync either way.

brendankenny · 2020-06-04T21:32:43Z

lighthouse-core/lib/page-functions.js

+ * declaration statement. Args should match the args of `mainFn`, and can be any serializable
+ * value. `deps` are functions that must be defined for `mainFn` to work.
+ */
+function createEvalCode(mainFn, {mode, args, deps} = {}) {


could this be directly incorporated into evaluate() (and evaluateFunctionOnObject) instead of splitting it into this file? It's mostly not shared code since the two callers take different branches in the conditional (and it's not really a "page function" in the same way as the other functions in this file are).

I prefer it here bc/ it's easier to test (driver has some overhead with beforeEach etc.) and
keeps the driver file smaller (it's so big). Maybe lib/eval.js?

And then export two function: createFunctionEvalCode and createIffeEvalCode?

I prefer it here bc/ it's easier to test (driver has some overhead with beforeEach etc.) and
keeps the driver file smaller (it's so big). Maybe lib/eval.js?

And then export two function: createFunctionEvalCode and createIffeEvalCode?

well it would definitely be good to split the two, but it'll also have a new home after #11633, and it also looks trivial to add to .evaluateAsync in driver-test?

const {expression} = connectionStub.sendCommand.findInvocation('Runtime.evaluate'); expect(expression).toEqual(`(() => { function mainFn() { return true; } return mainFn(); })()`);

@patrickhulce would you prefer this going into driver.js or a new file lib/eval.js?

new lib/eval.js file SGTM, though I'll also +1 @brendankenny's test suggest to make sure .evaluate uses it :)

new lib/eval.js file

Isn't the (currently named) RuntimeController going to be this already in #11633, though? I guess I don't understand what the problem is just inlining the five lines or so :)

Isn't the (currently named) RuntimeController going to be this already in #11633, though?

Ya, that's why I don't really care much where it lives in this PR :)

I guess I don't understand what the problem is just inlining the five lines or so

I mean I don't feel terribly strongly about it, the rebase gets slightly more annoying with inline but not by much.

connorjclark · 2020-11-13T01:21:21Z

saving for separate pr

return `function () {
  ${depsSerialized}
  ${mainFn}
  return ${mainFn.name}.call(this, ${argsSerialized});
}`;

----

it('function', () => {
  function mainFn(a, b) {
    return adder(a, b);
  }
  function adder(a, b) {
    return a + b;
  }
  const code = createEvalCode(mainFn, {
    args: [1, 2],
    mode: 'function',
    deps: [adder],
  });
  expect(code).toEqual(`function () {
    function adder(a, b) {
    return a + b;
  }
    function mainFn(a, b) {
    return adder(a, b);
  }
    return mainFn.call(this, 1,2);
  }`);
  expect(eval(`(${code})()`)).toEqual(3);
});

brendankenny

LGTM

brendankenny · 2020-11-13T16:18:17Z

lighthouse-core/test/gather/driver-test.js

+        });
+      }())`.trim();
+    expect(expression).toBe(expected);
+    expect(await eval(expression)).toBe(1);


with this eval idea you're introducing we could probably add some tests that exercise the error catching some time

cc @patrickhulce :)

my hope is that everything in driver becomes less scary and more obvious to test well when it's not all together :)

lighthouse-core/test/gather/driver-test.js

brendankenny · 2020-11-13T16:24:40Z

lighthouse-core/test/gather/driver-test.js

+
+    /**
+     * @param {{a: number, b: number}} _
+     * @param {any} passThru


no need to change, but I'm curious if there's a reason to make this any?

nothing happens to it, so the most applicable type is any.

lighthouse-core/test/gather/driver-test.js

connorjclark · 2020-11-13T19:58:45Z

oof, globalThis requires node 12. #11656

well, this has been open for 544 days (I really thought I opened this much more recently...woah), so what's a few more :)

patrickhulce · 2020-11-17T22:14:37Z

well, this has been open for 544 days (I really thought I opened this much more recently...woah), so what's a few more :)

This and #11633 will have quite the merge conundrum to sort out :) Are you planning on landing this imminently?

brendankenny · 2020-11-17T22:22:23Z

This and #11633 will have quite the merge conundrum to sort out :) Are you planning on landing this imminently?

shouldn't it be not too hard? It's just the one function now and the tests are a separate block (.evaluate() instead of .evaluateAsync())

patrickhulce · 2020-11-17T22:32:40Z

shouldn't it be not too hard? It's just the one function now and the tests are a separate block (.evaluate() instead of .evaluateAsync())

I mean sure, no PRs of reasonable size are ever really "too hard", but on the spectrum from normal Lighthouse PR with no merge conflicts to literally every line has a conflict, we're closer to the latter if we assume .evaluate logic moves into the controller as well.

connorjclark · 2020-11-17T22:44:01Z

Blocked on #11656 for usage of globalThis in our tests. Guess I'll get the honors of resolving the merge conflict.

patrickhulce · 2020-11-18T22:53:34Z

Guess I'll get the honors of resolving the merge conflict.

Sorry bud 😞

Don't worry though, I hear it's not too hard 😉

brendankenny · 2020-11-19T00:20:58Z

Don't worry though, I hear it's not too hard 😉

it's not automatic, but it's not hard :P

copy evaluate()
git merge -X theirs master
paste evaluate() back

everything is good!

move evaluate() to executionContext

connorjclark · 2020-12-01T23:47:51Z

not sure what to do with the jsdocs. 100% duplicate in both places?

core(driver): create page code using structured interface

90018b2

connorjclark requested a review from a team as a code owner May 20, 2020 01:39

connorjclark requested review from paulirish and removed request for a team May 20, 2020 01:39

googlebot added the cla: yes label May 20, 2020

devtools-bot assigned paulirish May 20, 2020

devtools-bot added the waiting4reviewer label May 20, 2020

connorjclark changed the title ~~core(driver): create page code using structured interface~~ core(driver): create eval code using structured interface May 20, 2020

connorjclark changed the title ~~core(driver): create eval code using structured interface~~ core(driver): create eval code using interface May 20, 2020

brendankenny reviewed May 20, 2020

View reviewed changes

connorjclark added 2 commits May 20, 2020 12:24

rename type

f7e38f5

pr feedback

78f386a

vercel bot deployed to Preview May 20, 2020 19:55 View deployment

connorjclark commented May 20, 2020

View reviewed changes

no strings

32856e7

vercel bot deployed to Preview May 20, 2020 22:18 View deployment

test

e2dd63d

vercel bot deployed to Preview May 20, 2020 22:41 View deployment

paulirish requested review from brendankenny and removed request for paulirish May 28, 2020 23:32

paulirish assigned brendankenny and unassigned paulirish May 28, 2020

connorjclark added 2 commits June 3, 2020 19:33

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

f38bf41

test

ce9839b

vercel bot deployed to Preview June 4, 2020 02:35 View deployment

brendankenny reviewed Jun 4, 2020

View reviewed changes

vercel bot deployed to Preview November 13, 2020 00:10 View deployment

inline

6be38eb

vercel bot deployed to Preview November 13, 2020 00:58 View deployment

brendankenny approved these changes Nov 13, 2020

View reviewed changes

last bits

97b0525

vercel bot deployed to Preview November 13, 2020 19:42 View deployment

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

81ea5b5

vercel bot deployed to Preview November 13, 2020 19:44 View deployment

adamraine removed their assignment Nov 16, 2020

brendankenny added upstream-action-required and removed waiting4reviewer labels Nov 18, 2020

Merge remote-tracking branch 'origin/master' into driver-eval-enhanced

4843291

vercel bot deployed to Preview December 1, 2020 23:47 View deployment

connorjclark removed the upstream-action-required label Dec 1, 2020

This comment has been minimized.

Sign in to view

tests

3c9f573

vercel bot deployed to Preview December 2, 2020 00:56 View deployment

connorjclark merged commit 46859c5 into master Dec 2, 2020

connorjclark deleted the driver-eval-enhanced branch December 2, 2020 01:18

connorjclark mentioned this pull request Dec 2, 2020

core: replace most usages of evaluateAsync with structured evaluate #11754

Merged

brendankenny mentioned this pull request Jul 15, 2021

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

Merged

core(driver): create eval code using interface #10816

core(driver): create eval code using interface #10816

Conversation

connorjclark commented May 20, 2020

brendankenny 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented May 20, 2020

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 commented May 20, 2020 • edited Loading

connorjclark commented Jun 4, 2020

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

Choose a reason for hiding this comment

connorjclark commented Nov 13, 2020

brendankenny 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

connorjclark commented Nov 13, 2020 • edited Loading

patrickhulce commented Nov 17, 2020

brendankenny commented Nov 17, 2020

patrickhulce commented Nov 17, 2020

connorjclark commented Nov 17, 2020

patrickhulce commented Nov 18, 2020

brendankenny commented Nov 19, 2020

connorjclark commented Dec 1, 2020

This comment has been minimized.

brendankenny commented May 20, 2020 •

edited

Loading

connorjclark commented Nov 13, 2020 •

edited

Loading