core(runner): asset manager helper #13519

adamraine · 2021-12-20T20:40:54Z

This PR kills two birds with one stone:

Accomplishes step 1 of Gather/Audit Mode in User Flows #13364. In a future PR, we can run the new helper function directly in navigation/timespan/snapshot/legacy runners.
Eliminates the url parameter for Runner.run. The URL should be passed into Runner.run as part of the gatherFn closure. This makes it easier to define a navigation runner that does not know the requested URL ahead of time core(fr): user triggered navigations #13496.

adamraine · 2021-12-20T20:43:25Z

lighthouse-core/runner.js

-        if (runOpts.url && !URL.equalWithExcludedFragments(runOpts.url, requestedUrl)) {
-          throw new Error('Cannot run audit mode on different URL');
-        }


Removing this check should be the only functional change to legacy mode. I didn't think it was needed because we can proceed with the requestedUrl from the artifacts. I'd be open to adding this back as a non-fatal warning or something in a different location.

The check makes more sense from the context of the CLI. Currently you can do -G and then -A separately but you must use the same URL, otherwise it errors because you probably made a mistake. Perhaps instead, we should have -A mode only use what is saved in the gathering step–url, settings, etc. and throw if a url is also provided

I would prefer just logging a warning, since we could just ignore the passed in URL.

i'm fine with just a warning. -G/A is pretty advanced usage so we don't really need these runtime failures to protect us, IMO

A report warning, then, not logger. This will otherwise be ignored and is sure to mess me up eventually so a loud warning is necessary :)

I would prefer switching to a fatal error in the CLI than that. We would need to send the warning message all the way down to Runner.run where run warnings are collected. Removing URL checks from Runner.run is part of the reason I created this PR.

The warning is quiet, but Lighthouse is still performing the more correct and expected operation IMO.

lighthouse-core/gather/driver/navigation.js

connorjclark · 2021-12-20T20:57:43Z

lighthouse-core/runner.js

-        if (runOpts.url && !URL.equalWithExcludedFragments(runOpts.url, requestedUrl)) {
-          throw new Error('Cannot run audit mode on different URL');
-        }


The check makes more sense from the context of the CLI. Currently you can do -G and then -A separately but you must use the same URL, otherwise it errors because you probably made a mistake. Perhaps instead, we should have -A mode only use what is saved in the gathering step–url, settings, etc. and throw if a url is also provided

paulirish

Eliminates the url parameter for Runner.run. The URL should be passed into Runner.run as part of the gatherFn closure.

I didnt totally understand the motivation here.. but the user-triggered nav usecase is definitely different. still don't totally understand it... cuz there, we don't even have a requestedUrl at all.. but... that's why we have this new 'requestor' thing in 13496. makes sense.

lighthouse-core/runner.js

paulirish · 2021-12-22T20:08:44Z

lighthouse-core/runner.js

-        if (runOpts.url && !URL.equalWithExcludedFragments(runOpts.url, requestedUrl)) {
-          throw new Error('Cannot run audit mode on different URL');
-        }


i'm fine with just a warning. -G/A is pretty advanced usage so we don't really need these runtime failures to protect us, IMO

brendankenny · 2021-12-22T21:23:03Z

Accomplishes step 1 of Gather/Audit Mode in User Flows #13364. In a future PR, we can run the new helper function directly in navigation/timespan/snapshot/legacy runners.

Moving the URL tracking to the artifacts make sense, but can you explain what this enables? All those modes already call into Runner.run() so already support gatherMode/auditMode.

The extracted code is also not independent of the other code in run(). e.g. if gatherAndManageAssets is called from another file, that file will also need the catch block for localizing LHErrors:

lighthouse/lighthouse-core/runner.js

Lines 177 to 184 in a26c8ee

    
           } catch (err) { 
        
             // i18n LighthouseError strings. 
        
             if (err.friendlyMessage) { 
        
               err.friendlyMessage = format.getFormatted(err.friendlyMessage, settings.locale); 
        
             } 
        
             await Sentry.captureException(err, {level: 'fatal'}); 
        
             throw err; 
        
           }

Part of what #12393 ran into in making an audit-runner is that there still needs to be a top-level orchestrator of some sort. We're always going to have UX niceties or issues with our conceptual model that will need to be handled somewhere and that prevent a pure functional artifacts -> audits -> LHR pipeline. Whether or not changing the URL for -A should be a runWarning, piping down runWarnings when a mode might not make a runWarnings array is a good example of the difficulty of an approach of "every mode just call these four functions in a row". Localizing error messages and reporting exceptions are other good examples. These things can all live somewhere, but eventually there's so many implicit ties between the four functions they might as well be called from a single place.

lighthouse-core/test/fraggle-rock/gather/mock-driver.js

lighthouse-core/test/runner-test.js

adamraine · 2021-12-22T22:16:17Z

Moving the URL tracking to the artifacts make sense, but can you explain what this enables?

Fewer string -> Function | string cases to handle in core(fr): user triggered navigations #13496
When gather phase and audit phase are separated, we won't need to backfill the url param for the audit phase.

The extracted code is also not independent of the other code in run(). e.g. if gatherAndManageAssets is called from another file, that file will also need the catch block for localizing LHErrors

This PR is reorganizing for future work in #13364, handling the dependencies is a part of that future work. We can duplicate the catch block and put it inside gatherAndManageAssets to address your example.

Part of what #12393 ran into in making an audit-runner is that there still needs to be a top-level orchestrator of some sort.

The goal of #13364 is not to get rid of the top-level orchestrator. The goal is to give the orchestrator some flexibility in when auditing and gathering happen so we can run gathering phases of all steps before running any auditing phases.

brendankenny · 2022-01-06T00:14:35Z

The goal of #13364 is not to get rid of the top-level orchestrator. The goal is to give the orchestrator some flexibility in when auditing and gathering happen so we can run gathering phases of all steps before running any auditing phases.

I think I see where you're coming from now. IIUC:

move from run(gatherFn, runOpts) to run(artifacts, runOpts), so artifacts need to be gathered or loaded from disk before runner.run is called, and
classic mode and navigation-runner need to share this functionality

Hence the new function.

(the quoted statement above was a little confusing because a user flow won't be able to use gatherAndManageArtifacts, but I finally got it that it will eventually be using run(artifacts, runOpts))

FWIW, this PR may have benefited from being split up with one stone per bird :) Both URL normalization and artifact acquisition are fairly touchy pipelines, and gatherAndManageArtifacts would be easier to review (e.g. for how defensive it needs to be) on its own (and ideally in context of how it will actually be called, though that's trickier).

Overall the change sounds good to me. I'm very excited about moving to a more explicit audit runner (#12393 🥲). I am a little concerned about the extra layers of test mocks (and intricacies of those mocks), but it does seem like a decent bit will be able to be removed as #13364 progresses? e.g. snapshot and timespan runners will be able ditch their mocked runners and only mock gathering, runner won't have to care about mocks at all anymore, etc

lighthouse-core/gather/driver/navigation.js

lighthouse-core/runner.js

connorjclark · 2022-01-06T20:32:56Z

lighthouse-core/test/fraggle-rock/gather/mock-driver.js

+    reset,
+  };
+
+  jest.mock('../../../runner.js', () => runnerModule);


these mock commands are tricky when it comes to es modules, b/c jest hoists them to the top to make them work. https://jestjs.io/docs/manual-mocks#using-with-es-module-imports

can keep it as-is since still commonjs, or just continue to do the jest.mock at the toplevel but instead like

jest.mock('....', createRunnerModuleMock()) // not even sure if this expression with a function call can be hoisted by jest 👀

no preference atm.

I like this way since it follows the format of mockDriverSubmodules.

adamraine · 2022-01-06T20:59:49Z

e.g. snapshot and timespan runners will be able ditch their mocked runners and only mock gathering, runner won't have to care about mocks at all anymore, etc

I don't know if #13364 will decrease the number of mocked modules, we might just change what's being mocked. Haven't investigated that much so we can cross that bridge later.

adamraine · 2022-01-06T21:41:06Z

lighthouse-core/lib/url-shim.js

@@ -10,6 +10,7 @@
 */

 const {Util} = require('../util-commonjs.js');
+const LHError = require('../lib/lh-error.js');


So turns out this creates a circular dependency that messes with our DT bundle. url-shim.js might not be the best place for this @brendankenny.

runner-helpers.js maybe?

brendankenny · 2022-01-06T21:54:01Z

is it the lighthouse-core/lib/i18n/i18n.js -> root.js -> lighthouse-core/lib/url-shim.js -> lighthouse-core/lib/lh-error.js -> lighthouse-core/lib/i18n/i18n.js message? Why is root.js depending on url-shim.js? Something seems messed up. root.js should be independent of the rest of Lighthouse or it's a real issue depending on it in so many places

brendankenny · 2022-01-06T22:00:58Z

looks like from here?

lighthouse/build/build-bundle.js

Lines 141 to 147 in da73d4a

    
           rollupPlugins.alias({ 
        
             entries: { 
        
               'debug': require.resolve('debug/src/browser.js'), 
        
               'lighthouse-logger': require.resolve('../lighthouse-logger/index.js'), 
        
               'url': require.resolve('../lighthouse-core/lib/url-shim.js'), 
        
             }, 
        
           }),

substituting our class derived from the global URL as the the 'url' node module doesn't seem necessary for anything (we always do an explicit url-shim import to use the extra functions) and I don't think it would work in root.js anyways (there's no fileURLToPath on the url-shim export, so would throw here if any devtools code called the readJson function after converting to esmodules)

adamraine · 2022-01-06T22:11:42Z

Removing that alias produces this in our DT tests:

I guess the alias is for pubads then?

brendankenny · 2022-01-06T23:55:50Z

lol: #5293 (review)

brendankenny · 2022-01-07T01:02:42Z

#13545 to fix url shimming for good

core: asset manager helper

552fe50

adamraine requested a review from a team as a code owner December 20, 2021 20:40

adamraine requested review from connorjclark and removed request for a team December 20, 2021 20:40

devtools-bot assigned connorjclark Dec 20, 2021

devtools-bot added the waiting4reviewer label Dec 20, 2021

adamraine commented Dec 20, 2021

View reviewed changes

connorjclark reviewed Dec 20, 2021

View reviewed changes

adamraine added 4 commits December 20, 2021 16:29

tests

739a058

comment

48cf364

warn

2f011d1

Merge branch 'master' into gather-asset-function

71c67e4

vercel bot deployed to Preview December 20, 2021 21:52 View deployment

just

e32c855

vercel bot deployed to Preview December 20, 2021 22:08 View deployment

adamraine changed the title ~~core: asset manager helper~~ core(runner): asset manager helper Dec 21, 2021

paulirish reviewed Dec 22, 2021

View reviewed changes

sentry

972bf83

vercel bot deployed to Preview December 22, 2021 20:26 View deployment

rn

8abf951

vercel bot deployed to Preview December 22, 2021 21:12 View deployment

brendankenny reviewed Dec 22, 2021

View reviewed changes

lighthouse-core/test/fraggle-rock/gather/mock-driver.js Outdated Show resolved Hide resolved

lighthouse-core/test/runner-test.js Show resolved Hide resolved

mock

7111b21

vercel bot deployed to Preview December 22, 2021 23:10 View deployment

adamraine added 2 commits January 5, 2022 17:15

Merge branch 'master' into gather-asset-function

b8a2da3

fix mocks

d073017

vercel bot deployed to Preview January 5, 2022 22:31 View deployment

invalid url test

07ef5d9

vercel bot deployed to Preview January 5, 2022 23:07 View deployment

brendankenny reviewed Jan 6, 2022

View reviewed changes

lighthouse-core/gather/driver/navigation.js Outdated Show resolved Hide resolved

url shim

382886d

vercel bot deployed to Preview January 6, 2022 01:04 View deployment

connorjclark approved these changes Jan 6, 2022

View reviewed changes

comments

fac8b10

vercel bot deployed to Preview January 6, 2022 20:46 View deployment

adamraine commented Jan 6, 2022

View reviewed changes

brendankenny mentioned this pull request Jan 7, 2022

clients: use minimal 'url' polyfil instead of url-shim #13545

Merged

Merge branch 'master' into gather-asset-function

b7f53f0

vercel bot deployed to Preview January 7, 2022 15:57 View deployment

adamraine merged commit 67c1d13 into master Jan 7, 2022

adamraine deleted the gather-asset-function branch January 7, 2022 17:10

adamraine mentioned this pull request Jan 20, 2022

Gather/Audit Mode in User Flows #13364

Closed

6 tasks

brendankenny mentioned this pull request Jan 20, 2022

core(runner): independent gather and audit functions #13569

Merged

This was referenced Jun 23, 2022

[Snyk] Security upgrade lighthouse from 8.6.0 to 9.3.0 MaxMood96/lighthouse#79

Open

[Snyk] Security upgrade lighthouse from 8.6.0 to 9.3.0 MaxMood96/lighthouse#80

Open

[Snyk] Security upgrade lighthouse from 8.6.0 to 9.3.0 MaxMood96/lighthouse#82

Open

ryan-ally mentioned this pull request Jun 21, 2023

[Snyk] Security upgrade lighthouse from 7.5.0 to 9.3.0 ryan-ally/lighthouse#30

Open

MaxMood96 mentioned this pull request Jun 22, 2023

[Snyk] Security upgrade lighthouse from 8.6.0 to 9.3.0 MaxMood96/lighthouse#94

Open

This was referenced Jun 23, 2023

[Snyk] Security upgrade lighthouse from 7.5.0 to 9.3.0 ryan-ally/lighthouse#32

Open

[Snyk] Security upgrade lighthouse from 7.5.0 to 9.3.0 ryan-ally/lighthouse#33

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(runner): asset manager helper #13519

core(runner): asset manager helper #13519

adamraine commented Dec 20, 2021

adamraine Dec 20, 2021

connorjclark Dec 20, 2021

adamraine Dec 20, 2021

paulirish Dec 22, 2021

connorjclark Dec 22, 2021

adamraine Dec 22, 2021 •

edited

Loading

connorjclark Dec 20, 2021

paulirish left a comment

paulirish Dec 22, 2021

brendankenny commented Dec 22, 2021

adamraine commented Dec 22, 2021

brendankenny commented Jan 6, 2022

connorjclark Jan 6, 2022

adamraine Jan 6, 2022

adamraine commented Jan 6, 2022

adamraine Jan 6, 2022

adamraine Jan 6, 2022

brendankenny commented Jan 6, 2022

brendankenny commented Jan 6, 2022

adamraine commented Jan 6, 2022 •

edited

Loading

brendankenny commented Jan 6, 2022

brendankenny commented Jan 7, 2022

core(runner): asset manager helper #13519

core(runner): asset manager helper #13519

Conversation

adamraine commented Dec 20, 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

adamraine Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Dec 22, 2021

adamraine commented Dec 22, 2021

brendankenny commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 6, 2022

brendankenny commented Jan 6, 2022

adamraine commented Jan 6, 2022 • edited Loading

brendankenny commented Jan 6, 2022

brendankenny commented Jan 7, 2022

adamraine Dec 22, 2021 •

edited

Loading

adamraine commented Jan 6, 2022 •

edited

Loading