From 8f11237a673e565550945020ef190f1d7376f898 Mon Sep 17 00:00:00 2001
From: jrandolf <101637635+jrandolf@users.noreply.github.com>
Date: Thu, 1 Sep 2022 17:09:57 +0200
Subject: [PATCH] chore: migrate `addScriptTag` (#8878)

---
 docs/api/puppeteer.frame.addscripttag.md      |   2 +-
 ...peteer.frameaddscripttagoptions.content.md |   2 +-
 .../puppeteer.frameaddscripttagoptions.id.md  |  15 +++
 .../api/puppeteer.frameaddscripttagoptions.md |  13 +-
 ...puppeteer.frameaddscripttagoptions.path.md |   2 +-
 ...puppeteer.frameaddscripttagoptions.type.md |   2 +-
 .../puppeteer.frameaddscripttagoptions.url.md |   2 +-
 docs/api/puppeteer.page.addscripttag.md       |  18 ++-
 src/common/Frame.ts                           |  82 +++++++++++--
 src/common/IsolatedWorld.ts                   | 111 +-----------------
 src/common/Page.ts                            |  33 +++---
 test/src/page.spec.ts                         |   6 +-
 12 files changed, 132 insertions(+), 156 deletions(-)
 create mode 100644 docs/api/puppeteer.frameaddscripttagoptions.id.md

diff --git a/docs/api/puppeteer.frame.addscripttag.md b/docs/api/puppeteer.frame.addscripttag.md
index 67e2638507e..c84a7c922ad 100644
--- a/docs/api/puppeteer.frame.addscripttag.md
+++ b/docs/api/puppeteer.frame.addscripttag.md
@@ -26,4 +26,4 @@ class Frame {
 
 Promise&lt;[ElementHandle](./puppeteer.elementhandle.md)&lt;HTMLScriptElement&gt;&gt;
 
-a promise that resolves to the added tag when the script's `onload` event fires or when the script content was injected into the frame.
+An [element handle](./puppeteer.elementhandle.md) to the injected `<script>` element.
diff --git a/docs/api/puppeteer.frameaddscripttagoptions.content.md b/docs/api/puppeteer.frameaddscripttagoptions.content.md
index bddf2a0bfb2..f9e3e79aa85 100644
--- a/docs/api/puppeteer.frameaddscripttagoptions.content.md
+++ b/docs/api/puppeteer.frameaddscripttagoptions.content.md
@@ -4,7 +4,7 @@ sidebar_label: FrameAddScriptTagOptions.content
 
 # FrameAddScriptTagOptions.content property
 
-Raw JavaScript content to be injected into the frame.
+JavaScript to be injected into the frame.
 
 **Signature:**
 
diff --git a/docs/api/puppeteer.frameaddscripttagoptions.id.md b/docs/api/puppeteer.frameaddscripttagoptions.id.md
new file mode 100644
index 00000000000..1b190494d3b
--- /dev/null
+++ b/docs/api/puppeteer.frameaddscripttagoptions.id.md
@@ -0,0 +1,15 @@
+---
+sidebar_label: FrameAddScriptTagOptions.id
+---
+
+# FrameAddScriptTagOptions.id property
+
+Sets the `id` of the script.
+
+**Signature:**
+
+```typescript
+interface FrameAddScriptTagOptions {
+  id?: string;
+}
+```
diff --git a/docs/api/puppeteer.frameaddscripttagoptions.md b/docs/api/puppeteer.frameaddscripttagoptions.md
index 8f76c855253..908a1de20ea 100644
--- a/docs/api/puppeteer.frameaddscripttagoptions.md
+++ b/docs/api/puppeteer.frameaddscripttagoptions.md
@@ -12,9 +12,10 @@ export interface FrameAddScriptTagOptions
 
 ## Properties
 
-| Property                                                    | Modifiers | Type   | Description                                                                                                      |
-| ----------------------------------------------------------- | --------- | ------ | ---------------------------------------------------------------------------------------------------------------- |
-| [content?](./puppeteer.frameaddscripttagoptions.content.md) |           | string | <i>(Optional)</i> Raw JavaScript content to be injected into the frame.                                          |
-| [path?](./puppeteer.frameaddscripttagoptions.path.md)       |           | string | <i>(Optional)</i> The path to a JavaScript file to be injected into the frame.                                   |
-| [type?](./puppeteer.frameaddscripttagoptions.type.md)       |           | string | <i>(Optional)</i> Set the script's <code>type</code>. Use <code>module</code> in order to load an ES2015 module. |
-| [url?](./puppeteer.frameaddscripttagoptions.url.md)         |           | string | <i>(Optional)</i> the URL of the script to be added.                                                             |
+| Property                                                    | Modifiers | Type   | Description                                                                                                            |
+| ----------------------------------------------------------- | --------- | ------ | ---------------------------------------------------------------------------------------------------------------------- |
+| [content?](./puppeteer.frameaddscripttagoptions.content.md) |           | string | <i>(Optional)</i> JavaScript to be injected into the frame.                                                            |
+| [id?](./puppeteer.frameaddscripttagoptions.id.md)           |           | string | <i>(Optional)</i> Sets the <code>id</code> of the script.                                                              |
+| [path?](./puppeteer.frameaddscripttagoptions.path.md)       |           | string | <i>(Optional)</i> Path to a JavaScript file to be injected into the frame.                                             |
+| [type?](./puppeteer.frameaddscripttagoptions.type.md)       |           | string | <i>(Optional)</i> Sets the <code>type</code> of the script. Use <code>module</code> in order to load an ES2015 module. |
+| [url?](./puppeteer.frameaddscripttagoptions.url.md)         |           | string | <i>(Optional)</i> URL of the script to be added.                                                                       |
diff --git a/docs/api/puppeteer.frameaddscripttagoptions.path.md b/docs/api/puppeteer.frameaddscripttagoptions.path.md
index 3c4cc734b2e..4a6af74cee7 100644
--- a/docs/api/puppeteer.frameaddscripttagoptions.path.md
+++ b/docs/api/puppeteer.frameaddscripttagoptions.path.md
@@ -4,7 +4,7 @@ sidebar_label: FrameAddScriptTagOptions.path
 
 # FrameAddScriptTagOptions.path property
 
-The path to a JavaScript file to be injected into the frame.
+Path to a JavaScript file to be injected into the frame.
 
 **Signature:**
 
diff --git a/docs/api/puppeteer.frameaddscripttagoptions.type.md b/docs/api/puppeteer.frameaddscripttagoptions.type.md
index 21d169aae2e..7c4c9e00665 100644
--- a/docs/api/puppeteer.frameaddscripttagoptions.type.md
+++ b/docs/api/puppeteer.frameaddscripttagoptions.type.md
@@ -4,7 +4,7 @@ sidebar_label: FrameAddScriptTagOptions.type
 
 # FrameAddScriptTagOptions.type property
 
-Set the script's `type`. Use `module` in order to load an ES2015 module.
+Sets the `type` of the script. Use `module` in order to load an ES2015 module.
 
 **Signature:**
 
diff --git a/docs/api/puppeteer.frameaddscripttagoptions.url.md b/docs/api/puppeteer.frameaddscripttagoptions.url.md
index 5ba0e8f33a3..f7d62fc0dcc 100644
--- a/docs/api/puppeteer.frameaddscripttagoptions.url.md
+++ b/docs/api/puppeteer.frameaddscripttagoptions.url.md
@@ -4,7 +4,7 @@ sidebar_label: FrameAddScriptTagOptions.url
 
 # FrameAddScriptTagOptions.url property
 
-the URL of the script to be added.
+URL of the script to be added.
 
 **Signature:**
 
diff --git a/docs/api/puppeteer.page.addscripttag.md b/docs/api/puppeteer.page.addscripttag.md
index 68eff996245..8c17b442a1e 100644
--- a/docs/api/puppeteer.page.addscripttag.md
+++ b/docs/api/puppeteer.page.addscripttag.md
@@ -10,27 +10,23 @@ Adds a `<script>` tag into the page with the desired URL or content.
 
 ```typescript
 class Page {
-  addScriptTag(options: {
-    url?: string;
-    path?: string;
-    content?: string;
-    type?: string;
-    id?: string;
-  }): Promise<ElementHandle<HTMLScriptElement>>;
+  addScriptTag(
+    options: FrameAddScriptTagOptions
+  ): Promise<ElementHandle<HTMLScriptElement>>;
 }
 ```
 
 ## Parameters
 
-| Parameter | Type                                                                           | Description |
-| --------- | ------------------------------------------------------------------------------ | ----------- |
-| options   | { url?: string; path?: string; content?: string; type?: string; id?: string; } |             |
+| Parameter | Type                                                                | Description             |
+| --------- | ------------------------------------------------------------------- | ----------------------- |
+| options   | [FrameAddScriptTagOptions](./puppeteer.frameaddscripttagoptions.md) | Options for the script. |
 
 **Returns:**
 
 Promise&lt;[ElementHandle](./puppeteer.elementhandle.md)&lt;HTMLScriptElement&gt;&gt;
 
-Promise which resolves to the added tag when the script's onload fires or when the script content was injected into frame.
+An [element handle](./puppeteer.elementhandle.md) to the injected `<script>` element.
 
 ## Remarks
 
diff --git a/src/common/Frame.ts b/src/common/Frame.ts
index 5eb99cc1584..bb992908275 100644
--- a/src/common/Frame.ts
+++ b/src/common/Frame.ts
@@ -49,24 +49,29 @@ export interface FrameWaitForFunctionOptions {
  */
 export interface FrameAddScriptTagOptions {
   /**
-   * the URL of the script to be added.
+   * URL of the script to be added.
    */
   url?: string;
   /**
-   * The path to a JavaScript file to be injected into the frame.
+   * Path to a JavaScript file to be injected into the frame.
+   *
    * @remarks
    * If `path` is a relative path, it is resolved relative to the current
    * working directory (`process.cwd()` in Node.js).
    */
   path?: string;
   /**
-   * Raw JavaScript content to be injected into the frame.
+   * JavaScript to be injected into the frame.
    */
   content?: string;
   /**
-   * Set the script's `type`. Use `module` in order to load an ES2015 module.
+   * Sets the `type` of the script. Use `module` in order to load an ES2015 module.
    */
   type?: string;
+  /**
+   * Sets the `id` of the script.
+   */
+  id?: string;
 }
 
 /**
@@ -736,14 +741,75 @@ export class Frame {
    * Adds a `<script>` tag into the page with the desired url or content.
    *
    * @param options - Options for the script.
-   * @returns a promise that resolves to the added tag when the script's
-   * `onload` event fires or when the script content was injected into the
-   * frame.
+   * @returns An {@link ElementHandle | element handle} to the injected
+   * `<script>` element.
    */
   async addScriptTag(
     options: FrameAddScriptTagOptions
   ): Promise<ElementHandle<HTMLScriptElement>> {
-    return this.worlds[MAIN_WORLD].addScriptTag(options);
+    let {content = '', type} = options;
+    const {path} = options;
+    if (+!!options.url + +!!path + +!!content !== 1) {
+      throw new Error(
+        'Exactly one of `url`, `path`, or `content` may be specified.'
+      );
+    }
+
+    if (path) {
+      let fs;
+      try {
+        fs = (await import('fs')).promises;
+      } catch (error) {
+        if (error instanceof TypeError) {
+          throw new Error(
+            'Can only pass a file path in a Node-like environment.'
+          );
+        }
+        throw error;
+      }
+      content = await fs.readFile(path, 'utf8');
+      content += `//# sourceURL=${path.replace(/\n/g, '')}`;
+    }
+
+    type = type ?? 'text/javascript';
+
+    return this.worlds[MAIN_WORLD].transferHandle(
+      await this.worlds[PUPPETEER_WORLD].evaluateHandle(
+        async ({url, id, type, content}) => {
+          const script = document.createElement('script');
+          script.type = type;
+          script.text = content;
+          if (url) {
+            script.src = url;
+          }
+          if (id) {
+            script.id = id;
+          }
+          let resolve: undefined | ((value?: unknown) => void);
+          const promise = new Promise((res, rej) => {
+            if (url) {
+              script.addEventListener('load', res, {once: true});
+            } else {
+              resolve = res;
+            }
+            script.addEventListener(
+              'error',
+              event => {
+                rej(event.message ?? 'Could not load script');
+              },
+              {once: true}
+            );
+          });
+          document.head.appendChild(script);
+          if (resolve) {
+            resolve();
+          }
+          await promise;
+          return script;
+        },
+        {...options, type, content}
+      )
+    );
   }
 
   /**
diff --git a/src/common/IsolatedWorld.ts b/src/common/IsolatedWorld.ts
index ff52fb95252..0b93dae9c5b 100644
--- a/src/common/IsolatedWorld.ts
+++ b/src/common/IsolatedWorld.ts
@@ -117,7 +117,7 @@ export class IsolatedWorld {
   #frame: Frame;
   #injected: boolean;
   #document?: ElementHandle<Document>;
-  #contextPromise = createDeferredPromise<ExecutionContext>();
+  #context = createDeferredPromise<ExecutionContext>();
   #detached = false;
 
   // Set of bindings that have been registered in the current context.
@@ -165,7 +165,7 @@ export class IsolatedWorld {
 
   clearContext(): void {
     this.#document = undefined;
-    this.#contextPromise = createDeferredPromise();
+    this.#context = createDeferredPromise();
   }
 
   setContext(context: ExecutionContext): void {
@@ -173,14 +173,14 @@ export class IsolatedWorld {
       context.evaluate(injectedSource).catch(debugError);
     }
     this.#ctxBindings.clear();
-    this.#contextPromise.resolve(context);
+    this.#context.resolve(context);
     for (const waitTask of this._waitTasks) {
       waitTask.rerun();
     }
   }
 
   hasContext(): boolean {
-    return this.#contextPromise.resolved();
+    return this.#context.resolved();
   }
 
   _detach(): void {
@@ -199,10 +199,10 @@ export class IsolatedWorld {
         `Execution context is not available in detached frame "${this.#frame.url()}" (are you trying to evaluate?)`
       );
     }
-    if (this.#contextPromise === null) {
+    if (this.#context === null) {
       throw new Error(`Execution content promise is missing`);
     }
-    return this.#contextPromise;
+    return this.#context;
   }
 
   async evaluateHandle<
@@ -334,105 +334,6 @@ export class IsolatedWorld {
     }
   }
 
-  /**
-   * Adds a script tag into the current context.
-   *
-   * @remarks
-   * You can pass a URL, filepath or string of contents. Note that when running Puppeteer
-   * in a browser environment you cannot pass a filepath and should use either
-   * `url` or `content`.
-   */
-  async addScriptTag(options: {
-    url?: string;
-    path?: string;
-    content?: string;
-    id?: string;
-    type?: string;
-  }): Promise<ElementHandle<HTMLScriptElement>> {
-    const {
-      url = null,
-      path = null,
-      content = null,
-      id = '',
-      type = '',
-    } = options;
-    if (url !== null) {
-      try {
-        const context = await this.executionContext();
-        return await context.evaluateHandle(addScriptUrl, url, id, type);
-      } catch (error) {
-        throw new Error(`Loading script from ${url} failed`);
-      }
-    }
-
-    if (path !== null) {
-      let fs;
-      try {
-        fs = (await import('fs')).promises;
-      } catch (error) {
-        if (error instanceof TypeError) {
-          throw new Error(
-            'Can only pass a filepath to addScriptTag in a Node-like environment.'
-          );
-        }
-        throw error;
-      }
-      let contents = await fs.readFile(path, 'utf8');
-      contents += '//# sourceURL=' + path.replace(/\n/g, '');
-      const context = await this.executionContext();
-      return await context.evaluateHandle(addScriptContent, contents, id, type);
-    }
-
-    if (content !== null) {
-      const context = await this.executionContext();
-      return await context.evaluateHandle(addScriptContent, content, id, type);
-    }
-
-    throw new Error(
-      'Provide an object with a `url`, `path` or `content` property'
-    );
-
-    async function addScriptUrl(url: string, id: string, type: string) {
-      const script = document.createElement('script');
-      script.src = url;
-      if (id) {
-        script.id = id;
-      }
-      if (type) {
-        script.type = type;
-      }
-      const promise = new Promise((res, rej) => {
-        script.onload = res;
-        script.onerror = rej;
-      });
-      document.head.appendChild(script);
-      await promise;
-      return script;
-    }
-
-    function addScriptContent(
-      content: string,
-      id: string,
-      type = 'text/javascript'
-    ) {
-      const script = document.createElement('script');
-      script.type = type;
-      script.text = content;
-      if (id) {
-        script.id = id;
-      }
-      let error = null;
-      script.onerror = e => {
-        return (error = e);
-      };
-      document.head.appendChild(script);
-      if (error) {
-        throw error;
-      }
-      return script;
-    }
-  }
-
   /**
    * Adds a style tag into the current context.
    *
diff --git a/src/common/Page.ts b/src/common/Page.ts
index b4f187ec3fb..c760f6fef58 100644
--- a/src/common/Page.ts
+++ b/src/common/Page.ts
@@ -16,23 +16,28 @@
 
 import {Protocol} from 'devtools-protocol';
 import type {Readable} from 'stream';
-import {Accessibility} from './Accessibility.js';
 import {assert} from '../util/assert.js';
+import {
+  createDeferredPromise,
+  DeferredPromise,
+} from '../util/DeferredPromise.js';
+import {isErrorLike} from '../util/ErrorLike.js';
+import {Accessibility} from './Accessibility.js';
 import {Browser, BrowserContext} from './Browser.js';
 import {CDPSession, CDPSessionEmittedEvents} from './Connection.js';
 import {ConsoleMessage, ConsoleMessageType} from './ConsoleMessage.js';
 import {Coverage} from './Coverage.js';
 import {Dialog} from './Dialog.js';
-import {MAIN_WORLD, WaitForSelectorOptions} from './IsolatedWorld.js';
 import {ElementHandle} from './ElementHandle.js';
 import {EmulationManager} from './EmulationManager.js';
 import {EventEmitter, Handler} from './EventEmitter.js';
 import {FileChooser} from './FileChooser.js';
+import {Frame, FrameAddScriptTagOptions} from './Frame.js';
 import {FrameManager, FrameManagerEmittedEvents} from './FrameManager.js';
-import {Frame} from './Frame.js';
 import {HTTPRequest} from './HTTPRequest.js';
 import {HTTPResponse} from './HTTPResponse.js';
 import {Keyboard, Mouse, MouseButton, Touchscreen} from './Input.js';
+import {MAIN_WORLD, WaitForSelectorOptions} from './IsolatedWorld.js';
 import {JSHandle} from './JSHandle.js';
 import {PuppeteerLifeCycleEvent} from './LifecycleWatcher.js';
 import {
@@ -53,9 +58,9 @@ import {
   debugError,
   evaluationString,
   getExceptionMessage,
-  importFS,
   getReadableAsBuffer,
   getReadableFromProtocolStream,
+  importFS,
   isNumber,
   isString,
   pageBindingDeliverErrorString,
@@ -67,11 +72,6 @@ import {
   waitForEvent,
   waitWithTimeout,
 } from './util.js';
-import {isErrorLike} from '../util/ErrorLike.js';
-import {
-  createDeferredPromise,
-  DeferredPromise,
-} from '../util/DeferredPromise.js';
 import {WebWorker} from './WebWorker.js';
 
 /**
@@ -1412,16 +1412,13 @@ export class Page extends EventEmitter {
    * Shortcut for
    * {@link Frame.addScriptTag | page.mainFrame().addScriptTag(options)}.
    *
-   * @returns Promise which resolves to the added tag when the script's onload
-   * fires or when the script content was injected into frame.
+   * @param options - Options for the script.
+   * @returns An {@link ElementHandle | element handle} to the injected
+   * `<script>` element.
    */
-  async addScriptTag(options: {
-    url?: string;
-    path?: string;
-    content?: string;
-    type?: string;
-    id?: string;
-  }): Promise<ElementHandle<HTMLScriptElement>> {
+  async addScriptTag(
+    options: FrameAddScriptTagOptions
+  ): Promise<ElementHandle<HTMLScriptElement>> {
     return this.mainFrame().addScriptTag(options);
   }
 
diff --git a/test/src/page.spec.ts b/test/src/page.spec.ts
index f350fa9bc90..d7620546cae 100644
--- a/test/src/page.spec.ts
+++ b/test/src/page.spec.ts
@@ -1631,7 +1631,7 @@ describe('Page', function () {
         error = error_ as Error;
       }
       expect(error.message).toBe(
-        'Provide an object with a `url`, `path` or `content` property'
+        'Exactly one of `url`, `path`, or `content` may be specified.'
       );
     });
 
@@ -1702,7 +1702,7 @@ describe('Page', function () {
       } catch (error_) {
         error = error_ as Error;
       }
-      expect(error.message).toBe('Loading script from /nonexistfile.js failed');
+      expect(error.message).toContain('Could not load script');
     });
 
     it('should work with a path', async () => {
@@ -1850,7 +1850,7 @@ describe('Page', function () {
         path: path.join(__dirname, '../assets/injectedstyle.css'),
       });
       const styleHandle = (await page.$('style'))!;
-      const styleContent = await page.evaluate((style: HTMLStyleElement) => {
+      const styleContent = await page.evaluate(style => {
         return style.innerHTML;
       }, styleHandle);
       expect(styleContent).toContain(path.join('assets', 'injectedstyle.css'));