fix: remove import cycle in connection (#11225)

packages/puppeteer-core/src/bidi/Connection.ts

@@ -16,7 +16,7 @@
 
 import type * as Bidi from 'chromium-bidi/lib/cjs/protocol/protocol.js';
 
-import {CallbackRegistry} from '../cdp/Connection.js';
+import {CallbackRegistry} from '../common/CallbackRegistry.js';
 import type {ConnectionTransport} from '../common/ConnectionTransport.js';
 import {debug} from '../common/Debug.js';
 import {EventEmitter} from '../common/EventEmitter.js';

packages/puppeteer-core/src/cdp/CDPSession.ts

@@ -1,3 +1,19 @@
+/**
+ * Copyright 2017 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 import type {ProtocolMapping} from 'devtools-protocol/types/protocol-mapping.js';
 
 import {
@@ -5,14 +21,12 @@ import {
   CDPSession,
   CDPSessionEvent,
 } from '../api/CDPSession.js';
+import {CallbackRegistry} from '../common/CallbackRegistry.js';
 import {TargetCloseError} from '../common/Errors.js';
 import {assert} from '../util/assert.js';
+import {createProtocolErrorMessage} from '../util/ErrorLike.js';
 
-import {
-  CallbackRegistry,
-  type Connection,
-  createProtocolErrorMessage,
-} from './Connection.js';
+import type {Connection} from './Connection.js';
 import type {CdpTarget} from './Target.js';
 
 /**

packages/puppeteer-core/src/cdp/Connection.ts

@@ -22,12 +22,12 @@ import {
   type CDPSession,
   type CDPSessionEvents,
 } from '../api/CDPSession.js';
+import {CallbackRegistry} from '../common/CallbackRegistry.js';
 import type {ConnectionTransport} from '../common/ConnectionTransport.js';
 import {debug} from '../common/Debug.js';
-import {ProtocolError, TargetCloseError} from '../common/Errors.js';
+import {TargetCloseError} from '../common/Errors.js';
 import {EventEmitter} from '../common/EventEmitter.js';
-import {debugError} from '../common/util.js';
-import {Deferred} from '../util/Deferred.js';
+import {createProtocolErrorMessage} from '../util/ErrorLike.js';
 
 import {CdpCDPSession} from './CDPSession.js';
 
@@ -39,159 +39,6 @@ const debugProtocolReceive = debug('puppeteer:protocol:RECV ◀');
  */
 export type {ConnectionTransport, ProtocolMapping};
 
-/**
- * @internal
- */
-type GetIdFn = () => number;
-
-/**
- * @internal
- */
-function createIncrementalIdGenerator(): GetIdFn {
-  let id = 0;
-  return (): number => {
-    return ++id;
-  };
-}
-
-/**
- * @internal
- */
-export class Callback {
-  #id: number;
-  #error = new ProtocolError();
-  #deferred = Deferred.create<unknown>();
-  #timer?: ReturnType<typeof setTimeout>;
-  #label: string;
-
-  constructor(id: number, label: string, timeout?: number) {
-    this.#id = id;
-    this.#label = label;
-    if (timeout) {
-      this.#timer = setTimeout(() => {
-        this.#deferred.reject(
-          rewriteError(
-            this.#error,
-            `${label} timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.`
-          )
-        );
-      }, timeout);
-    }
-  }
-
-  resolve(value: unknown): void {
-    clearTimeout(this.#timer);
-    this.#deferred.resolve(value);
-  }
-
-  reject(error: Error): void {
-    clearTimeout(this.#timer);
-    this.#deferred.reject(error);
-  }
-
-  get id(): number {
-    return this.#id;
-  }
-
-  get promise(): Deferred<unknown> {
-    return this.#deferred;
-  }
-
-  get error(): ProtocolError {
-    return this.#error;
-  }
-
-  get label(): string {
-    return this.#label;
-  }
-}
-
-/**
- * Manages callbacks and their IDs for the protocol request/response communication.
- *
- * @internal
- */
-export class CallbackRegistry {
-  #callbacks = new Map<number, Callback>();
-  #idGenerator = createIncrementalIdGenerator();
-
-  create(
-    label: string,
-    timeout: number | undefined,
-    request: (id: number) => void
-  ): Promise<unknown> {
-    const callback = new Callback(this.#idGenerator(), label, timeout);
-    this.#callbacks.set(callback.id, callback);
-    try {
-      request(callback.id);
-    } catch (error) {
-      // We still throw sync errors synchronously and clean up the scheduled
-      // callback.
-      callback.promise
-        .valueOrThrow()
-        .catch(debugError)
-        .finally(() => {
-          this.#callbacks.delete(callback.id);
-        });
-      callback.reject(error as Error);
-      throw error;
-    }
-    // Must only have sync code up until here.
-    return callback.promise.valueOrThrow().finally(() => {
-      this.#callbacks.delete(callback.id);
-    });
-  }
-
-  reject(id: number, message: string, originalMessage?: string): void {
-    const callback = this.#callbacks.get(id);
-    if (!callback) {
-      return;
-    }
-    this._reject(callback, message, originalMessage);
-  }
-
-  _reject(
-    callback: Callback,
-    errorMessage: string | ProtocolError,
-    originalMessage?: string
-  ): void {
-    let error: ProtocolError;
-    let message: string;
-    if (errorMessage instanceof ProtocolError) {
-      error = errorMessage;
-      error.cause = callback.error;
-      message = errorMessage.message;
-    } else {
-      error = callback.error;
-      message = errorMessage;
-    }
-
-    callback.reject(
-      rewriteError(
-        error,
-        `Protocol error (${callback.label}): ${message}`,
-        originalMessage
-      )
-    );
-  }
-
-  resolve(id: number, value: unknown): void {
-    const callback = this.#callbacks.get(id);
-    if (!callback) {
-      return;
-    }
-    callback.resolve(value);
-  }
-
-  clear(): void {
-    for (const callback of this.#callbacks.values()) {
-      // TODO: probably we can accept error messages as params.
-      this._reject(callback, new TargetCloseError('Target closed'));
-    }
-    this.#callbacks.clear();
-  }
-}
-
 /**
  * @public
  */
@@ -414,35 +261,6 @@ export class Connection extends EventEmitter<CDPSessionEvents> {
   }
 }
 
-/**
- * @internal
- */
-export function createProtocolErrorMessage(object: {
-  error: {message: string; data: any; code: number};
-}): string {
-  let message = `${object.error.message}`;
-  // TODO: remove the type checks when we stop connecting to BiDi with a CDP
-  // client.
-  if (
-    object.error &&
-    typeof object.error === 'object' &&
-    'data' in object.error
-  ) {
-    message += ` ${object.error.data}`;
-  }
-  return message;
-}
-
-function rewriteError(
-  error: ProtocolError,
-  message: string,
-  originalMessage?: string
-): Error {
-  error.message = message;
-  error.originalMessage = originalMessage ?? error.originalMessage;
-  return error;
-}
-
 /**
  * @internal
  */

packages/puppeteer-core/src/common/CallbackRegistry.ts

@@ -0,0 +1,174 @@
+/**
+ * Copyright 2023 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import {Deferred} from '../util/Deferred.js';
+import {rewriteError} from '../util/ErrorLike.js';
+
+import {ProtocolError, TargetCloseError} from './Errors.js';
+import {debugError} from './util.js';
+
+/**
+ * Manages callbacks and their IDs for the protocol request/response communication.
+ *
+ * @internal
+ */
+export class CallbackRegistry {
+  #callbacks = new Map<number, Callback>();
+  #idGenerator = createIncrementalIdGenerator();
+
+  create(
+    label: string,
+    timeout: number | undefined,
+    request: (id: number) => void
+  ): Promise<unknown> {
+    const callback = new Callback(this.#idGenerator(), label, timeout);
+    this.#callbacks.set(callback.id, callback);
+    try {
+      request(callback.id);
+    } catch (error) {
+      // We still throw sync errors synchronously and clean up the scheduled
+      // callback.
+      callback.promise
+        .valueOrThrow()
+        .catch(debugError)
+        .finally(() => {
+          this.#callbacks.delete(callback.id);
+        });
+      callback.reject(error as Error);
+      throw error;
+    }
+    // Must only have sync code up until here.
+    return callback.promise.valueOrThrow().finally(() => {
+      this.#callbacks.delete(callback.id);
+    });
+  }
+
+  reject(id: number, message: string, originalMessage?: string): void {
+    const callback = this.#callbacks.get(id);
+    if (!callback) {
+      return;
+    }
+    this._reject(callback, message, originalMessage);
+  }
+
+  _reject(
+    callback: Callback,
+    errorMessage: string | ProtocolError,
+    originalMessage?: string
+  ): void {
+    let error: ProtocolError;
+    let message: string;
+    if (errorMessage instanceof ProtocolError) {
+      error = errorMessage;
+      error.cause = callback.error;
+      message = errorMessage.message;
+    } else {
+      error = callback.error;
+      message = errorMessage;
+    }
+
+    callback.reject(
+      rewriteError(
+        error,
+        `Protocol error (${callback.label}): ${message}`,
+        originalMessage
+      )
+    );
+  }
+
+  resolve(id: number, value: unknown): void {
+    const callback = this.#callbacks.get(id);
+    if (!callback) {
+      return;
+    }
+    callback.resolve(value);
+  }
+
+  clear(): void {
+    for (const callback of this.#callbacks.values()) {
+      // TODO: probably we can accept error messages as params.
+      this._reject(callback, new TargetCloseError('Target closed'));
+    }
+    this.#callbacks.clear();
+  }
+}
+/**
+ * @internal
+ */
+
+export class Callback {
+  #id: number;
+  #error = new ProtocolError();
+  #deferred = Deferred.create<unknown>();
+  #timer?: ReturnType<typeof setTimeout>;
+  #label: string;
+
+  constructor(id: number, label: string, timeout?: number) {
+    this.#id = id;
+    this.#label = label;
+    if (timeout) {
+      this.#timer = setTimeout(() => {
+        this.#deferred.reject(
+          rewriteError(
+            this.#error,
+            `${label} timed out. Increase the 'protocolTimeout' setting in launch/connect calls for a higher timeout if needed.`
+          )
+        );
+      }, timeout);
+    }
+  }
+
+  resolve(value: unknown): void {
+    clearTimeout(this.#timer);
+    this.#deferred.resolve(value);
+  }
+
+  reject(error: Error): void {
+    clearTimeout(this.#timer);
+    this.#deferred.reject(error);
+  }
+
+  get id(): number {
+    return this.#id;
+  }
+
+  get promise(): Deferred<unknown> {
+    return this.#deferred;
+  }
+
+  get error(): ProtocolError {
+    return this.#error;
+  }
+
+  get label(): string {
+    return this.#label;
+  }
+}
+
+/**
+ * @internal
+ */
+export function createIncrementalIdGenerator(): GetIdFn {
+  let id = 0;
+  return (): number => {
+    return ++id;
+  };
+}
+
+/**
+ * @internal
+ */
+export type GetIdFn = () => number;

packages/puppeteer-core/src/util/ErrorLike.ts

@@ -1,27 +1,76 @@
+/**
+ * Copyright 2022 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type {ProtocolError} from '../common/Errors.js';
+
 /**
  * @internal
  */
-
 export interface ErrorLike extends Error {
   name: string;
   message: string;
 }
+
 /**
  * @internal
  */
-
 export function isErrorLike(obj: unknown): obj is ErrorLike {
   return (
     typeof obj === 'object' && obj !== null && 'name' in obj && 'message' in obj
   );
 }
+
 /**
  * @internal
  */
-
 export function isErrnoException(obj: unknown): obj is NodeJS.ErrnoException {
   return (
     isErrorLike(obj) &&
     ('errno' in obj || 'code' in obj || 'path' in obj || 'syscall' in obj)
   );
 }
+
+/**
+ * @internal
+ */
+export function rewriteError(
+  error: ProtocolError,
+  message: string,
+  originalMessage?: string
+): Error {
+  error.message = message;
+  error.originalMessage = originalMessage ?? error.originalMessage;
+  return error;
+}
+
+/**
+ * @internal
+ */
+export function createProtocolErrorMessage(object: {
+  error: {message: string; data: any; code: number};
+}): string {
+  let message = `${object.error.message}`;
+  // TODO: remove the type checks when we stop connecting to BiDi with a CDP
+  // client.
+  if (
+    object.error &&
+    typeof object.error === 'object' &&
+    'data' in object.error
+  ) {
+    message += ` ${object.error.data}`;
+  }
+  return message;
+}