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(driver): add sendCommandAndIgnore #15913

Merged
merged 5 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const throwingSession = {
once: throwNotConnectedFn,
off: throwNotConnectedFn,
sendCommand: throwNotConnectedFn,
sendCommandAndIgnore: throwNotConnectedFn,
dispose: throwNotConnectedFn,
};

Expand Down
2 changes: 1 addition & 1 deletion core/gather/driver/target-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ class TargetManager extends ProtocolEventEmitter {
throw err;
} finally {
// Resume the target if it was paused, but if it's unnecessary, we don't care about the error.
await newSession.sendCommand('Runtime.runIfWaitingForDebugger').catch(() => {});
await newSession.sendCommandAndIgnore('Runtime.runIfWaitingForDebugger');
}
}

Expand Down
5 changes: 2 additions & 3 deletions core/gather/driver/wait-for-condition.js
Original file line number Diff line number Diff line change
Expand Up @@ -498,9 +498,8 @@ async function waitForFullyLoaded(session, networkMonitor, options) {
if (await isPageHung(session)) {
log.warn('waitFor', 'Page appears to be hung, killing JavaScript...');
// We don't await these, as we want to exit with PAGE_HUNG
void session.sendCommand('Emulation.setScriptExecutionDisabled', {value: true})
.catch(_ => {});
void session.sendCommand('Runtime.terminateExecution').catch(_ => {});
void session.sendCommandAndIgnore('Emulation.setScriptExecutionDisabled', {value: true});
void session.sendCommandAndIgnore('Runtime.terminateExecution');
Copy link
Collaborator

@connorjclark connorjclark Apr 3, 2024

Choose a reason for hiding this comment

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

Thoughts on making this return void, doing the void * hack inside the method?

if the semantics is just "toss this over to cdp and move on" we don't need to await anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm yeah. i'm up for tweaking it.

the void * thing (generally a convention from the devtools codebase) is for saying.. i'm not awaiting or returning this promise. https://typescript-eslint.io/rules/no-floating-promises/#ignorevoid

but the new AndIgnore method is for.. catching the rejection so it doesn't continue (or go unhandled)

they're slightly different things, right?

and it seems like we have cases where we want to await the AndIgnore calls .. and cases where we don't want to await them
So i think this new method should still return a promise. but.. it can be empty


and given this tweak, maybe ignore isn't a great fit. wdyt sendCommandQuietly? open to other names. (but also no big whoop on naming)

throw new LighthouseError(LighthouseError.errors.PAGE_HUNG);
}

Expand Down
12 changes: 12 additions & 0 deletions core/gather/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,18 @@ class ProtocolSession extends CrdpEventEmitter {
});
}

/**
* Send and if there's an error response, do not reject.
* @template {keyof LH.CrdpCommands} C
* @param {C} method
* @param {LH.CrdpCommands[C]['paramsType']} params
* @return {Promise<LH.CrdpCommands[C]['returnType']>}
*/
sendCommandAndIgnore(method, ...params) {
return this.sendCommand(method, ...params)
.catch(e => log.verbose('session', method, e.message));
}

/**
* Disposes of a session so that it can no longer talk to Chrome.
* @return {Promise<void>}
Expand Down
4 changes: 3 additions & 1 deletion core/test/gather/mock-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ import {NetworkMonitor} from '../../gather/driver/network-monitor.js';
/** @typedef {import('../../gather/driver/execution-context.js')} ExecutionContext */

function createMockSession() {
const mockSendCommand = createMockSendCommandFn();
return {
setTargetInfo: fnAny(),
sendCommand: createMockSendCommandFn(),
sendCommand: mockSendCommand,
sendCommandAndIgnore: mockSendCommand,
setNextProtocolTimeout: fnAny(),
hasNextProtocolTimeout: fnAny(),
getNextProtocolTimeout: fnAny(),
Expand Down
1 change: 1 addition & 0 deletions types/gatherer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ declare module Gatherer {
once<TEvent extends keyof CrdpEvents>(event: TEvent, callback: (...args: CrdpEvents[TEvent]) => void): void;
off<TEvent extends keyof CrdpEvents>(event: TEvent, callback: (...args: CrdpEvents[TEvent]) => void): void;
sendCommand<TMethod extends keyof CrdpCommands>(method: TMethod, ...params: CrdpCommands[TMethod]['paramsType']): Promise<CrdpCommands[TMethod]['returnType']>;
sendCommandAndIgnore<TMethod extends keyof CrdpCommands>(method: TMethod, ...params: CrdpCommands[TMethod]['paramsType']): Promise<CrdpCommands[TMethod]['returnType']>;
dispose(): Promise<void>;
}

Expand Down
Loading