Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(fr): user triggered navigations #13496

Merged
merged 40 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
f3c652a
start
adamraine Oct 8, 2021
e0f15c5
redirect
adamraine Oct 12, 2021
02edaec
Merge branch 'master' into fr-navigation-requestor
adamraine Oct 12, 2021
95c7f08
single func
adamraine Oct 12, 2021
69fd6be
Merge branch 'master' into fr-navigation-requestor
adamraine Nov 5, 2021
dd1351e
Merge branch 'master' into fr-navigation-requestor
adamraine Nov 15, 2021
70d48ac
hacks
adamraine Nov 17, 2021
ee18ff8
Merge branch 'master' into fr-navigation-requestor
adamraine Dec 13, 2021
9ae0f48
revert pdg
adamraine Dec 13, 2021
8b555ca
revert flow
adamraine Dec 13, 2021
9114f2b
Merge branch 'master' into fr-navigation-requestor
adamraine Dec 13, 2021
6ad4ddd
rm wait for func
adamraine Dec 13, 2021
63ccd8e
rm splitter
adamraine Dec 13, 2021
7ef0265
fix types
adamraine Dec 13, 2021
d6da3d1
up
adamraine Dec 13, 2021
2109c36
Merge branch 'master' into fr-navigation-requestor
adamraine Jan 7, 2022
b121b26
no sample rebaseline
adamraine Jan 7, 2022
a412b0f
network monitor test
adamraine Jan 10, 2022
a9e395f
prepare test
adamraine Jan 10, 2022
afdcc0e
navigation test
adamraine Jan 10, 2022
634d00f
navigation runner test
adamraine Jan 10, 2022
8e4acb1
api test
adamraine Jan 10, 2022
5f43e57
Merge branch 'master' into fr-navigation-requestor
adamraine Jan 10, 2022
5147683
Merge branch 'master' into fr-navigation-requestor
adamraine Feb 8, 2022
dfb6515
Merge branch 'master' into fr-navigation-requestor
adamraine Feb 9, 2022
f3ffa05
Merge branch 'fr-navigation-requestor' of github.com:GoogleChrome/lig…
adamraine Feb 9, 2022
eb93948
test
adamraine Feb 9, 2022
b0cdd14
rn
adamraine Feb 9, 2022
ea40068
docs
adamraine Feb 9, 2022
c3b3a4a
docs
adamraine Feb 9, 2022
277d424
test
adamraine Feb 10, 2022
076094f
doc
adamraine Feb 10, 2022
87d141a
nit
adamraine Feb 10, 2022
3a98544
api change
adamraine Feb 10, 2022
29f6b1a
auto enable skipAboutBlank
adamraine Feb 10, 2022
80c4134
rn
adamraine Feb 10, 2022
46b4b55
url
adamraine Feb 10, 2022
a8c1543
Merge branch 'master' into fr-navigation-requestor
adamraine Feb 10, 2022
4950b16
error msg
adamraine Feb 10, 2022
658b54e
word
adamraine Feb 10, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ async function runLighthouseWithFraggleRock(url, flags, config, launchedChrome)
const page = await browser.newPage();
flags.channel = 'fraggle-rock-cli';
const configContext = {configPath: flags.configPath, settingsOverrides: flags};
return fraggleRock.navigation({url, page, config, configContext});
return fraggleRock.navigation({requestor: url, page, config, configContext});
}

/**
Expand Down
64 changes: 39 additions & 25 deletions lighthouse-core/fraggle-rock/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const NetworkRecords = require('../../computed/network-records.js');
* @property {Driver} driver
* @property {LH.Config.FRConfig} config
* @property {LH.Config.NavigationDefn} navigation
* @property {string} requestedUrl
* @property {LH.NavigationRequestor} requestor
* @property {LH.FRBaseArtifacts} baseArtifacts
* @property {Map<string, LH.ArbitraryEqualityMap>} computedCache
* @property {InternalOptions} [options]
Expand All @@ -44,17 +44,17 @@ const NetworkRecords = require('../../computed/network-records.js');
/** @typedef {Omit<Parameters<typeof collectPhaseArtifacts>[0], 'phase'>} PhaseState */

/**
* @param {{driver: Driver, config: LH.Config.FRConfig, requestedUrl: string, options?: InternalOptions}} args
* @param {{driver: Driver, config: LH.Config.FRConfig, requestor: LH.NavigationRequestor, options?: InternalOptions}} args
* @return {Promise<{baseArtifacts: LH.FRBaseArtifacts}>}
*/
async function _setup({driver, config, requestedUrl, options}) {
async function _setup({driver, config, requestor, options}) {
await driver.connect();
if (!options?.skipAboutBlank) {
await gotoURL(driver, defaultNavigationConfig.blankPage, {waitUntil: ['navigated']});
}

const baseArtifacts = await getBaseArtifacts(config, driver, {gatherMode: 'navigation'});
baseArtifacts.URL.requestedUrl = requestedUrl;
if (typeof requestor === 'string') baseArtifacts.URL.requestedUrl = requestor;

await prepare.prepareTargetForNavigationMode(driver, config.settings);

Expand All @@ -65,7 +65,7 @@ async function _setup({driver, config, requestedUrl, options}) {
* @param {NavigationContext} navigationContext
* @return {Promise<{warnings: Array<LH.IcuMessage>}>}
*/
async function _setupNavigation({requestedUrl, driver, navigation, config, options}) {
async function _setupNavigation({requestor, driver, navigation, config, options}) {
if (!options?.skipAboutBlank) {
await gotoURL(driver, navigation.blankPage, {...navigation, waitUntil: ['navigated']});
}
Expand All @@ -74,7 +74,7 @@ async function _setupNavigation({requestedUrl, driver, navigation, config, optio
config.settings,
{
...navigation,
url: requestedUrl,
requestor,
}
);

Expand All @@ -90,25 +90,25 @@ async function _cleanupNavigation({driver}) {

/**
* @param {NavigationContext} navigationContext
* @return {Promise<{finalUrl: string, navigationError: LH.LighthouseError | undefined, warnings: Array<LH.IcuMessage>}>}
* @return {Promise<{requestedUrl: string, finalUrl: string, navigationError: LH.LighthouseError | undefined, warnings: Array<LH.IcuMessage>}>}
*/
async function _navigate(navigationContext) {
const {driver, config, requestedUrl} = navigationContext;
const {driver, config, requestor} = navigationContext;

try {
const {finalUrl, warnings} = await gotoURL(driver, requestedUrl, {
const {requestedUrl, finalUrl, warnings} = await gotoURL(driver, requestor, {
...navigationContext.navigation,
debugNavigation: config.settings.debugNavigation,
maxWaitForFcp: config.settings.maxWaitForFcp,
maxWaitForLoad: config.settings.maxWaitForLoad,
waitUntil: navigationContext.navigation.pauseAfterFcpMs ? ['fcp', 'load'] : ['load'],
});

return {finalUrl, navigationError: undefined, warnings};
return {requestedUrl, finalUrl, navigationError: undefined, warnings};
} catch (err) {
if (!(err instanceof LighthouseError)) throw err;
if (err.code !== 'NO_FCP' && err.code !== 'PAGE_HUNG') throw err;
return {finalUrl: requestedUrl, navigationError: err, warnings: []};
if (typeof requestor !== 'string') throw err;
return {requestedUrl: requestor, finalUrl: requestor, navigationError: err, warnings: []};
}
}

Expand Down Expand Up @@ -152,7 +152,7 @@ async function _collectDebugData(navigationContext, phaseState) {
* @param {PhaseState} phaseState
* @param {Awaited<ReturnType<typeof _setupNavigation>>} setupResult
* @param {Awaited<ReturnType<typeof _navigate>>} navigateResult
* @return {Promise<{finalUrl: string, artifacts: Partial<LH.GathererArtifacts>, warnings: Array<LH.IcuMessage>, pageLoadError: LH.LighthouseError | undefined}>}
* @return {Promise<{requestedUrl: string, finalUrl: string, artifacts: Partial<LH.GathererArtifacts>, warnings: Array<LH.IcuMessage>, pageLoadError: LH.LighthouseError | undefined}>}
*/
async function _computeNavigationResult(
navigationContext,
Expand All @@ -174,7 +174,7 @@ async function _computeNavigationResult(
if (pageLoadError) {
const locale = navigationContext.config.settings.locale;
const localizedMessage = format.getFormatted(pageLoadError.friendlyMessage, locale);
log.error('NavigationRunner', localizedMessage, navigationContext.requestedUrl);
log.error('NavigationRunner', localizedMessage, navigateResult.requestedUrl);

/** @type {Partial<LH.GathererArtifacts>} */
const artifacts = {};
Expand All @@ -183,6 +183,7 @@ async function _computeNavigationResult(
if (debugData.trace) artifacts.traces = {[pageLoadErrorId]: debugData.trace};

return {
requestedUrl: navigateResult.requestedUrl,
finalUrl,
pageLoadError,
artifacts,
Expand All @@ -192,7 +193,13 @@ async function _computeNavigationResult(
await collectPhaseArtifacts({phase: 'getArtifact', ...phaseState});

const artifacts = await awaitArtifacts(phaseState.artifactState);
return {finalUrl, artifacts, warnings, pageLoadError: undefined};
return {
requestedUrl: navigateResult.requestedUrl,
finalUrl,
artifacts,
warnings,
pageLoadError: undefined,
};
}
}

Expand Down Expand Up @@ -224,10 +231,10 @@ async function _navigation(navigationContext) {
}

/**
* @param {{driver: Driver, config: LH.Config.FRConfig, requestedUrl: string; baseArtifacts: LH.FRBaseArtifacts, computedCache: NavigationContext['computedCache'], options?: InternalOptions}} args
* @param {{driver: Driver, config: LH.Config.FRConfig, requestor: LH.NavigationRequestor; baseArtifacts: LH.FRBaseArtifacts, computedCache: NavigationContext['computedCache'], options?: InternalOptions}} args
* @return {Promise<{artifacts: Partial<LH.FRArtifacts & LH.FRBaseArtifacts>}>}
*/
async function _navigations({driver, config, requestedUrl, baseArtifacts, computedCache, options}) {
async function _navigations({driver, config, requestor, baseArtifacts, computedCache, options}) {
if (!config.navigations) throw new Error('No navigations configured');

/** @type {Partial<LH.FRArtifacts & LH.FRBaseArtifacts>} */
Expand All @@ -239,7 +246,7 @@ async function _navigations({driver, config, requestedUrl, baseArtifacts, comput
const navigationContext = {
driver,
navigation,
requestedUrl,
requestor,
config,
baseArtifacts,
computedCache,
Expand All @@ -254,7 +261,10 @@ async function _navigations({driver, config, requestedUrl, baseArtifacts, comput
shouldHaltNavigations = true;
}

artifacts.URL = {requestedUrl, finalUrl: navigationResult.finalUrl};
artifacts.URL = {
requestedUrl: navigationResult.requestedUrl,
finalUrl: navigationResult.finalUrl,
};
}

LighthouseRunWarnings.push(...navigationResult.warnings);
Expand All @@ -266,21 +276,21 @@ async function _navigations({driver, config, requestedUrl, baseArtifacts, comput
}

/**
* @param {{requestedUrl: string, driver: Driver, config: LH.Config.FRConfig}} args
* @param {{requestedUrl?: string, driver: Driver, config: LH.Config.FRConfig}} args
*/
async function _cleanup({requestedUrl, driver, config}) {
const didResetStorage = !config.settings.disableStorageReset;
const didResetStorage = !config.settings.disableStorageReset && requestedUrl;
if (didResetStorage) await storage.clearDataForOrigin(driver.defaultSession, requestedUrl);

await driver.disconnect();
}

/**
* @param {{url: string, page: import('puppeteer').Page, config?: LH.Config.Json, configContext?: LH.Config.FRContext}} options
* @param {{requestor: LH.NavigationRequestor, page: import('puppeteer').Page, config?: LH.Config.Json, configContext?: LH.Config.FRContext}} options
* @return {Promise<LH.RunnerResult|undefined>}
*/
async function navigation(options) {
const {url, page, configContext = {}} = options;
const {requestor, page, configContext = {}} = options;
const {config} = initializeConfig(options.config, {...configContext, gatherMode: 'navigation'});
const computedCache = new Map();
const internalOptions = {
Expand All @@ -291,8 +301,12 @@ async function navigation(options) {
const artifacts = await Runner.gather(
async () => {
const driver = new Driver(page);
const requestedUrl = URL.normalizeUrl(url);
const context = {driver, config, requestedUrl, options: internalOptions};
const context = {
driver,
config,
requestor: typeof requestor === 'string' ? URL.normalizeUrl(requestor) : requestor,
options: internalOptions,
};
const {baseArtifacts} = await _setup(context);
const {artifacts} = await _navigations({...context, baseArtifacts, computedCache});
await _cleanup(context);
Expand Down
12 changes: 6 additions & 6 deletions lighthouse-core/fraggle-rock/user-flow.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@ class UserFlow {
}

/**
* @param {string} url
* @param {LH.NavigationRequestor} requestor
* @param {StepOptions=} stepOptions
*/
_getNextNavigationOptions(url, stepOptions) {
const options = {url, ...this.options, ...stepOptions};
_getNextNavigationOptions(requestor, stepOptions) {
const options = {requestor, ...this.options, ...stepOptions};
const configContext = {...options.configContext};
const settingsOverrides = {...configContext.settingsOverrides};

Expand All @@ -81,13 +81,13 @@ class UserFlow {
}

/**
* @param {string} url
* @param {LH.NavigationRequestor} requestor
* @param {StepOptions=} stepOptions
*/
async navigate(url, stepOptions) {
async navigate(requestor, stepOptions) {
if (this.currentTimespan) throw Error('Timespan already in progress');

const result = await navigation(this._getNextNavigationOptions(url, stepOptions));
const result = await navigation(this._getNextNavigationOptions(requestor, stepOptions));
if (!result) throw Error('Navigation returned undefined');

const providedName = stepOptions?.stepName;
Expand Down
31 changes: 21 additions & 10 deletions lighthouse-core/gather/driver/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,14 @@ function resolveWaitForFullyLoadedOptions(options) {
* navigations.
*
* @param {LH.Gatherer.FRTransitionalDriver} driver
* @param {string} url
* @param {LH.NavigationRequestor} requestor
* @param {NavigationOptions} options
* @return {Promise<{finalUrl: string, warnings: Array<LH.IcuMessage>}>}
* @return {Promise<{requestedUrl: string, finalUrl: string, warnings: Array<LH.IcuMessage>}>}
*/
async function gotoURL(driver, url, options) {
const status = {msg: `Navigating to ${url}`, id: 'lh:driver:navigate'};
async function gotoURL(driver, requestor, options) {
const status = typeof requestor === 'string' ?
{msg: `Navigating to ${requestor}`, id: 'lh:driver:navigate'} :
{msg: 'Navigating using a user defined function', id: 'lh:driver:navigate'};
log.time(status);

const session = driver.defaultSession;
Expand All @@ -94,9 +96,14 @@ async function gotoURL(driver, url, options) {
await session.sendCommand('Page.enable');
await session.sendCommand('Page.setLifecycleEventsEnabled', {enabled: true});

// No timeout needed for Page.navigate. See https://github.com/GoogleChrome/lighthouse/pull/6413
session.setNextProtocolTimeout(Infinity);
const waitforPageNavigateCmd = session.sendCommand('Page.navigate', {url});
let waitForPageNavigateCmd;
adamraine marked this conversation as resolved.
Show resolved Hide resolved
if (typeof requestor === 'string') {
// No timeout needed for Page.navigate. See https://github.com/GoogleChrome/lighthouse/pull/6413
session.setNextProtocolTimeout(Infinity);
waitForPageNavigateCmd = session.sendCommand('Page.navigate', {url: requestor});
} else {
waitForPageNavigateCmd = requestor();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be useful to make a new status logging step here, and keep the original for this method the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

We will have to replace the URL in the original log message with something for user controlled navigations. I think that would make a log statement here redundant.

}

const waitForNavigated = options.waitUntil.includes('navigated');
const waitForLoad = options.waitUntil.includes('load');
Expand All @@ -119,10 +126,13 @@ async function gotoURL(driver, url, options) {

const waitConditions = await Promise.all(waitConditionPromises);
const timedOut = waitConditions.some(condition => condition.timedOut);
const finalUrl = (await networkMonitor.getFinalNavigationUrl()) || url;
const navigationUrls = await networkMonitor.getNavigationUrls();
const requestedUrl = typeof requestor === 'string' ? requestor : navigationUrls.requestedUrl;
if (!requestedUrl) throw Error('No navigations detected when running user defined requestor.');
const finalUrl = navigationUrls.finalUrl || requestedUrl;

// Bring `Page.navigate` errors back into the promise chain. See https://github.com/GoogleChrome/lighthouse/pull/6739.
await waitforPageNavigateCmd;
await waitForPageNavigateCmd;
await networkMonitor.disable();

if (options.debugNavigation) {
Expand All @@ -131,8 +141,9 @@ async function gotoURL(driver, url, options) {

log.timeEnd(status);
return {
requestedUrl,
finalUrl,
warnings: getNavigationWarnings({timedOut, finalUrl, requestedUrl: url}),
warnings: getNavigationWarnings({timedOut, finalUrl, requestedUrl}),
};
}

Expand Down
14 changes: 8 additions & 6 deletions lighthouse-core/gather/driver/network-monitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,18 +133,20 @@ class NetworkMonitor {
this._sessions = new Map();
}

/** @return {Promise<string | undefined>} */
async getFinalNavigationUrl() {
/** @return {Promise<{requestedUrl?: string, finalUrl?: string}>} */
async getNavigationUrls() {
const frameNavigations = this._frameNavigations;
if (!frameNavigations.length) return undefined;
if (!frameNavigations.length) return {};

const resourceTreeResponse = await this._session.sendCommand('Page.getResourceTree');
const mainFrameId = resourceTreeResponse.frameTree.frame.id;
const mainFrameNavigations = frameNavigations.filter(frame => frame.id === mainFrameId);
const finalNavigation = mainFrameNavigations[mainFrameNavigations.length - 1];
if (!finalNavigation) log.warn('NetworkMonitor', 'No detected navigations');
if (!mainFrameNavigations.length) log.warn('NetworkMonitor', 'No detected navigations');

return finalNavigation?.url;
return {
requestedUrl: mainFrameNavigations[0]?.url,
adamraine marked this conversation as resolved.
Show resolved Hide resolved
finalUrl: mainFrameNavigations[mainFrameNavigations.length - 1]?.url,
};
}

/**
Expand Down
18 changes: 11 additions & 7 deletions lighthouse-core/gather/driver/prepare.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,17 @@ async function dismissJavaScriptDialogs(session) {

/**
* @param {LH.Gatherer.FRProtocolSession} session
* @param {{url: string}} navigation
* @param {string} url
* @return {Promise<{warnings: Array<LH.IcuMessage>}>}
*/
async function resetStorageForNavigation(session, navigation) {
async function resetStorageForUrl(session, url) {
/** @type {Array<LH.IcuMessage>} */
const warnings = [];

// Reset the storage and warn if there appears to be other important data.
const warning = await storage.getImportantStorageWarning(session, navigation.url);
const warning = await storage.getImportantStorageWarning(session, url);
if (warning) warnings.push(warning);
await storage.clearDataForOrigin(session, navigation.url);
await storage.clearDataForOrigin(session, url);
await storage.clearBrowserCaches(session);

return {warnings};
Expand Down Expand Up @@ -190,7 +190,7 @@ async function prepareTargetForNavigationMode(driver, settings) {
*
* @param {LH.Gatherer.FRProtocolSession} session
* @param {LH.Config.Settings} settings
* @param {Pick<LH.Config.NavigationDefn, 'disableThrottling'|'disableStorageReset'|'blockedUrlPatterns'> & {url: string}} navigation
* @param {Pick<LH.Config.NavigationDefn, 'disableThrottling'|'disableStorageReset'|'blockedUrlPatterns'> & {requestor: LH.NavigationRequestor}} navigation
* @return {Promise<{warnings: Array<LH.IcuMessage>}>}
*/
async function prepareTargetForIndividualNavigation(session, settings, navigation) {
Expand All @@ -200,9 +200,13 @@ async function prepareTargetForIndividualNavigation(session, settings, navigatio
/** @type {Array<LH.IcuMessage>} */
const warnings = [];

const shouldResetStorage = !settings.disableStorageReset && !navigation.disableStorageReset;
const {requestor} = navigation;
const shouldResetStorage =
!settings.disableStorageReset &&
!navigation.disableStorageReset &&
typeof requestor === 'string';
adamraine marked this conversation as resolved.
Show resolved Hide resolved
if (shouldResetStorage) {
const {warnings: storageWarnings} = await resetStorageForNavigation(session, navigation);
const {warnings: storageWarnings} = await resetStorageForUrl(session, requestor);
adamraine marked this conversation as resolved.
Show resolved Hide resolved
warnings.push(...storageWarnings);
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ class GatherRunner {
driver.defaultSession,
passContext.settings,
{
url: passContext.url,
requestor: passContext.url,
disableStorageReset: !passConfig.useThrottling,
disableThrottling: !passConfig.useThrottling,
blockedUrlPatterns: passConfig.blockedUrlPatterns,
Expand Down
Loading