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

chore: pass explicit recorder app factory #32349

Merged
merged 1 commit into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all 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: 0 additions & 1 deletion packages/playwright-core/src/cli/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,6 @@ async function codegen(options: Options & { target: string, output?: string, tes
mode: 'recording',
testIdAttributeName,
outputFile: outputFile ? path.resolve(outputFile) : undefined,
handleSIGINT: false,
Copy link
Member

Choose a reason for hiding this comment

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

is it related to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I don't think we need the handleSIGINT: true mode for recorder, at all, so removing.

});
await openPage(context, url);
}
Expand Down
1 change: 0 additions & 1 deletion packages/playwright-core/src/client/browserContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,6 @@ export class BrowserContext extends ChannelOwner<channels.BrowserContextChannel>
mode?: 'recording' | 'inspecting',
testIdAttributeName?: string,
outputFile?: string,
handleSIGINT?: boolean,
}) {
await this._channel.recorderSupplementEnable(params);
}
Expand Down
1 change: 0 additions & 1 deletion packages/playwright-core/src/protocol/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,6 @@ scheme.BrowserContextRecorderSupplementEnableParams = tObject({
device: tOptional(tString),
saveStorage: tOptional(tString),
outputFile: tOptional(tString),
handleSIGINT: tOptional(tBoolean),
omitCallTracking: tOptional(tBoolean),
});
scheme.BrowserContextRecorderSupplementEnableResult = tOptional(tObject({}));
Expand Down
9 changes: 6 additions & 3 deletions packages/playwright-core/src/server/browserContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { BrowserContextAPIRequestContext } from './fetch';
import type { Artifact } from './artifact';
import { Clock } from './clock';
import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
import { RecorderApp } from './recorder/recorderApp';

export abstract class BrowserContext extends SdkObject {
static Events = {
Expand Down Expand Up @@ -130,13 +131,15 @@ export abstract class BrowserContext extends SdkObject {

// When PWDEBUG=1, show inspector for each context.
if (debugMode() === 'inspector')
await Recorder.show(this, { pauseOnNextStatement: true });
await Recorder.show(this, RecorderApp.factory(this), { pauseOnNextStatement: true });

// When paused, show inspector.
if (this._debugger.isPaused())
Recorder.showInspector(this);
Recorder.showInspector(this, RecorderApp.factory(this));

this._debugger.on(Debugger.Events.PausedStateChanged, () => {
Recorder.showInspector(this);
if (this._debugger.isPaused())
Recorder.showInspector(this, RecorderApp.factory(this));
});

if (debugMode() === 'console')
Expand Down
4 changes: 1 addition & 3 deletions packages/playwright-core/src/server/debugController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export class DebugController extends SdkObject {
initialize(codegenId: string, sdkLanguage: Language) {
this._codegenId = codegenId;
this._sdkLanguage = sdkLanguage;
Recorder.setAppFactory(async () => new InspectingRecorderApp(this));
}

setAutoCloseAllowed(allowed: boolean) {
Expand All @@ -62,7 +61,6 @@ export class DebugController extends SdkObject {
dispose() {
this.setReportStateChanged(false);
this.setAutoCloseAllowed(false);
Recorder.setAppFactory(undefined);
}

setReportStateChanged(enabled: boolean) {
Expand Down Expand Up @@ -199,7 +197,7 @@ export class DebugController extends SdkObject {
const contexts = new Set<BrowserContext>();
for (const page of this._playwright.allPages())
contexts.add(page.context());
const result = await Promise.all([...contexts].map(c => Recorder.show(c, { omitCallTracking: true })));
const result = await Promise.all([...contexts].map(c => Recorder.show(c, () => Promise.resolve(new InspectingRecorderApp(this)), { omitCallTracking: true })));
return result.filter(Boolean) as Recorder[];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import type { Dialog } from '../dialog';
import type { ConsoleMessage } from '../console';
import { serializeError } from '../errors';
import { ElementHandleDispatcher } from './elementHandlerDispatcher';
import { RecorderApp } from '../recorder/recorderApp';

export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channels.BrowserContextChannel, DispatcherScope> implements channels.BrowserContextChannel {
_type_EventTarget = true;
Expand Down Expand Up @@ -291,7 +292,7 @@ export class BrowserContextDispatcher extends Dispatcher<BrowserContext, channel
}

async recorderSupplementEnable(params: channels.BrowserContextRecorderSupplementEnableParams): Promise<void> {
await Recorder.show(this._context, params);
await Recorder.show(this._context, RecorderApp.factory(this._context), params);
}

async pause(params: channels.BrowserContextPauseParams, metadata: CallMetadata) {
Expand Down
42 changes: 17 additions & 25 deletions packages/playwright-core/src/server/recorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ import { type Language } from './codegen/types';
import { Debugger } from './debugger';
import type { CallMetadata, InstrumentationListener, SdkObject } from './instrumentation';
import { ContextRecorder, generateFrameSelector } from './recorder/contextRecorder';
import type { IRecorderApp } from './recorder/recorderApp';
import { EmptyRecorderApp, RecorderApp } from './recorder/recorderApp';
import { type IRecorderApp } from './recorder/recorderApp';
import { buildFullSelector, metadataToCallLog } from './recorder/recorderUtils';

const recorderSymbol = Symbol('recorderSymbol');

export type RecorderAppFactory = (recorder: Recorder) => Promise<IRecorderApp>;

export class Recorder implements InstrumentationListener {
private _context: BrowserContext;
private _mode: Mode;
Expand All @@ -43,40 +44,38 @@ export class Recorder implements InstrumentationListener {
private _userSources = new Map<string, Source>();
private _debugger: Debugger;
private _contextRecorder: ContextRecorder;
private _handleSIGINT: boolean | undefined;
private _omitCallTracking = false;
private _currentLanguage: Language;

private static recorderAppFactory: ((recorder: Recorder) => Promise<IRecorderApp>) | undefined;

static setAppFactory(recorderAppFactory: ((recorder: Recorder) => Promise<IRecorderApp>) | undefined) {
Recorder.recorderAppFactory = recorderAppFactory;
}

static showInspector(context: BrowserContext) {
static showInspector(context: BrowserContext, recorderAppFactory: RecorderAppFactory) {
const params: channels.BrowserContextRecorderSupplementEnableParams = {};
if (isUnderTest())
params.language = process.env.TEST_INSPECTOR_LANGUAGE;
Recorder.show(context, params).catch(() => {});
Recorder.show(context, recorderAppFactory, params).catch(() => {});
}

static show(context: BrowserContext, params: channels.BrowserContextRecorderSupplementEnableParams = {}): Promise<Recorder> {
static show(context: BrowserContext, recorderAppFactory: RecorderAppFactory, params: channels.BrowserContextRecorderSupplementEnableParams = {}): Promise<Recorder> {
let recorderPromise = (context as any)[recorderSymbol] as Promise<Recorder>;
if (!recorderPromise) {
const recorder = new Recorder(context, params);
recorderPromise = recorder.install().then(() => recorder);
recorderPromise = Recorder._create(context, recorderAppFactory, params);
(context as any)[recorderSymbol] = recorderPromise;
}
return recorderPromise;
}

private static async _create(context: BrowserContext, recorderAppFactory: RecorderAppFactory, params: channels.BrowserContextRecorderSupplementEnableParams = {}): Promise<Recorder> {
const recorder = new Recorder(context, params);
const recorderApp = await recorderAppFactory(recorder);
await recorder._install(recorderApp);
return recorder;
}

constructor(context: BrowserContext, params: channels.BrowserContextRecorderSupplementEnableParams) {
this._mode = params.mode || 'none';
this._contextRecorder = new ContextRecorder(context, params);
this._context = context;
this._omitCallTracking = !!params.omitCallTracking;
this._debugger = context.debugger();
this._handleSIGINT = params.handleSIGINT;
context.instrumentation.addListener(this, context);
this._currentLanguage = this._contextRecorder.languageName();

Expand All @@ -86,14 +85,7 @@ export class Recorder implements InstrumentationListener {
}
}

private static async defaultRecorderAppFactory(recorder: Recorder) {
if (process.env.PW_CODEGEN_NO_INSPECTOR)
return new EmptyRecorderApp();
return await RecorderApp.open(recorder, recorder._context, recorder._handleSIGINT);
}

async install() {
const recorderApp = await (Recorder.recorderAppFactory || Recorder.defaultRecorderAppFactory)(this);
private async _install(recorderApp: IRecorderApp) {
this._recorderApp = recorderApp;
recorderApp.once('close', () => {
this._debugger.resume(false);
Expand Down Expand Up @@ -140,7 +132,7 @@ export class Recorder implements InstrumentationListener {
this._context.once(BrowserContext.Events.Close, () => {
this._contextRecorder.dispose();
this._context.instrumentation.removeListener(this);
recorderApp.close().catch(() => {});
this._recorderApp?.close().catch(() => {});
});
this._contextRecorder.on(ContextRecorder.Events.Change, (data: { sources: Source[], primaryFileName: string }) => {
this._recorderSources = data.sources;
Expand Down Expand Up @@ -201,7 +193,7 @@ export class Recorder implements InstrumentationListener {
this._pausedStateChanged();
this._debugger.on(Debugger.Events.PausedStateChanged, () => this._pausedStateChanged());

(this._context as any).recorderAppForTest = recorderApp;
(this._context as any).recorderAppForTest = this._recorderApp;
}

_pausedStateChanged() {
Expand Down
18 changes: 13 additions & 5 deletions packages/playwright-core/src/server/recorder/recorderApp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import type { CallLog, EventData, Mode, Source } from '@recorder/recorderTypes';
import { isUnderTest } from '../../utils';
import { mime } from '../../utilsBundle';
import { syncLocalStorageWithSettings } from '../launchApp';
import type { Recorder } from '../recorder';
import type { Recorder, RecorderAppFactory } from '../recorder';
import type { BrowserContext } from '../browserContext';
import { launchApp } from '../launchApp';

Expand Down Expand Up @@ -113,7 +113,15 @@ export class RecorderApp extends EventEmitter implements IRecorderApp {
await mainFrame.goto(serverSideCallMetadata(), 'https://playwright/index.html');
}

static async open(recorder: Recorder, inspectedContext: BrowserContext, handleSIGINT: boolean | undefined): Promise<IRecorderApp> {
static factory(context: BrowserContext): RecorderAppFactory {
return async recorder => {
if (process.env.PW_CODEGEN_NO_INSPECTOR)
return new EmptyRecorderApp();
return await RecorderApp._open(recorder, context);
};
}

private static async _open(recorder: Recorder, inspectedContext: BrowserContext): Promise<IRecorderApp> {
const sdkLanguage = inspectedContext.attribution.playwright.options.sdkLanguage;
const headed = !!inspectedContext._browser.options.headful;
const recorderPlaywright = (require('../playwright').createPlaywright as typeof import('../playwright').createPlaywright)({ sdkLanguage: 'javascript', isInternalPlaywright: true });
Expand All @@ -125,7 +133,7 @@ export class RecorderApp extends EventEmitter implements IRecorderApp {
noDefaultViewport: true,
headless: !!process.env.PWTEST_CLI_HEADLESS || (isUnderTest() && !headed),
useWebSocket: !!process.env.PWTEST_RECORDER_PORT,
handleSIGINT,
handleSIGINT: false,
args: process.env.PWTEST_RECORDER_PORT ? [`--remote-debugging-port=${process.env.PWTEST_RECORDER_PORT}`] : [],
executablePath: inspectedContext._browser.options.isChromium ? inspectedContext._browser.options.customExecutablePath : undefined,
}
Expand Down Expand Up @@ -170,11 +178,11 @@ export class RecorderApp extends EventEmitter implements IRecorderApp {

async setSelector(selector: string, userGesture?: boolean): Promise<void> {
if (userGesture) {
if (this._recorder.mode() === 'inspecting') {
if (this._recorder?.mode() === 'inspecting') {
this._recorder.setMode('standby');
this._page.bringToFront();
} else {
this._recorder.setMode('recording');
this._recorder?.setMode('recording');
}
}
await this._page.mainFrame().evaluateExpression(((data: { selector: string, userGesture?: boolean }) => {
Expand Down
2 changes: 0 additions & 2 deletions packages/protocol/src/channels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1753,7 +1753,6 @@ export type BrowserContextRecorderSupplementEnableParams = {
device?: string,
saveStorage?: string,
outputFile?: string,
handleSIGINT?: boolean,
omitCallTracking?: boolean,
};
export type BrowserContextRecorderSupplementEnableOptions = {
Expand All @@ -1766,7 +1765,6 @@ export type BrowserContextRecorderSupplementEnableOptions = {
device?: string,
saveStorage?: string,
outputFile?: string,
handleSIGINT?: boolean,
omitCallTracking?: boolean,
};
export type BrowserContextRecorderSupplementEnableResult = void;
Expand Down
1 change: 0 additions & 1 deletion packages/protocol/src/protocol.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,6 @@ BrowserContext:
device: string?
saveStorage: string?
outputFile: string?
handleSIGINT: boolean?
omitCallTracking: boolean?

newCDPSession:
Expand Down
Loading