chore: break dowload.path() to throw (#27662)

docs/src/api/class-download.md

@@ -70,7 +70,7 @@ Upon successful cancellations, `download.failure()` would resolve to `'canceled'
 ## async method: Download.createReadStream
 * since: v1.8
 * langs: java, js, csharp
-- returns: <[null]|[Readable]>
+- returns: <[Readable]>
 
 Returns readable stream for current download or `null` if download failed.
 
@@ -93,7 +93,7 @@ Get the page that the download belongs to.
 
 ## async method: Download.path
 * since: v1.8
-- returns: <[null]|[path]>
+- returns: <[path]>
 
 Returns path to the downloaded file in case of successful download. The method will
 wait for the download to finish if necessary. The method throws when connected remotely.

packages/playwright-core/src/client/artifact.ts

@@ -26,10 +26,10 @@ export class Artifact extends ChannelOwner<channels.ArtifactChannel> {
     return (channel as any)._object;
   }
 
-  async pathAfterFinished(): Promise<string | null> {
+  async pathAfterFinished(): Promise<string> {
     if (this._connection.isRemote())
       throw new Error(`Path is not available when connecting remotely. Use saveAs() to save a local copy.`);
-    return (await this._channel.pathAfterFinished()).value || null;
+    return (await this._channel.pathAfterFinished()).value;
   }
 
   async saveAs(path: string): Promise<void> {
@@ -52,10 +52,8 @@ export class Artifact extends ChannelOwner<channels.ArtifactChannel> {
     return (await this._channel.failure()).error || null;
   }
 
-  async createReadStream(): Promise<Readable | null> {
+  async createReadStream(): Promise<Readable> {
     const result = await this._channel.stream();
-    if (!result.stream)
-      return null;
     const stream = Stream.from(result.stream);
     return stream.stream();
   }

packages/playwright-core/src/client/download.ts

@@ -44,7 +44,7 @@ export class Download implements api.Download {
     return this._suggestedFilename;
   }
 
-  async path(): Promise<string | null> {
+  async path(): Promise<string> {
     return this._artifact.pathAfterFinished();
   }
 
@@ -56,7 +56,7 @@ export class Download implements api.Download {
     return this._artifact.failure();
   }
 
-  async createReadStream(): Promise<Readable | null> {
+  async createReadStream(): Promise<Readable> {
     return this._artifact.createReadStream();
   }
 

packages/playwright-core/src/protocol/validator.ts

@@ -2161,7 +2161,7 @@ scheme.ArtifactInitializer = tObject({
 });
 scheme.ArtifactPathAfterFinishedParams = tOptional(tObject({}));
 scheme.ArtifactPathAfterFinishedResult = tObject({
-  value: tOptional(tString),
+  value: tString,
 });
 scheme.ArtifactSaveAsParams = tObject({
   path: tString,
@@ -2177,7 +2177,7 @@ scheme.ArtifactFailureResult = tObject({
 });
 scheme.ArtifactStreamParams = tOptional(tObject({}));
 scheme.ArtifactStreamResult = tObject({
-  stream: tOptional(tChannel(['Stream'])),
+  stream: tChannel(['Stream']),
 });
 scheme.ArtifactCancelParams = tOptional(tObject({}));
 scheme.ArtifactCancelResult = tOptional(tObject({}));

packages/playwright-core/src/server/artifact.ts

@@ -18,8 +18,9 @@ import fs from 'fs';
 import { assert } from '../utils';
 import { ManualPromise } from '../utils/manualPromise';
 import { SdkObject } from './instrumentation';
+import { TargetClosedError } from '../common/errors';
 
-type SaveCallback = (localPath: string, error?: string) => Promise<void>;
+type SaveCallback = (localPath: string, error?: Error) => Promise<void>;
 type CancelCallback = () => Promise<void>;
 
 export class Artifact extends SdkObject {
@@ -30,7 +31,7 @@ export class Artifact extends SdkObject {
   private _saveCallbacks: SaveCallback[] = [];
   private _finished: boolean = false;
   private _deleted = false;
-  private _failureError: string | null = null;
+  private _failureError: Error | undefined;
 
   constructor(parent: SdkObject, localPath: string, unaccessibleErrorMessage?: string, cancelCallback?: CancelCallback) {
     super(parent, 'artifact');
@@ -47,12 +48,12 @@ export class Artifact extends SdkObject {
     return this._localPath;
   }
 
-  async localPathAfterFinished(): Promise<string | null> {
+  async localPathAfterFinished(): Promise<string> {
     if (this._unaccessibleErrorMessage)
       throw new Error(this._unaccessibleErrorMessage);
     await this._finishedPromise;
     if (this._failureError)
-      return null;
+      throw this._failureError;
     return this._localPath;
   }
 
@@ -62,10 +63,10 @@ export class Artifact extends SdkObject {
     if (this._deleted)
       throw new Error(`File already deleted. Save before deleting.`);
     if (this._failureError)
-      throw new Error(`File not found on disk. Check download.failure() for details.`);
+      throw this._failureError;
 
     if (this._finished) {
-      saveCallback(this._localPath).catch(e => {});
+      saveCallback(this._localPath).catch(() => {});
       return;
     }
     this._saveCallbacks.push(saveCallback);
@@ -75,7 +76,7 @@ export class Artifact extends SdkObject {
     if (this._unaccessibleErrorMessage)
       return this._unaccessibleErrorMessage;
     await this._finishedPromise;
-    return this._failureError;
+    return this._failureError?.message || null;
   }
 
   async cancel(): Promise<void> {
@@ -102,14 +103,14 @@ export class Artifact extends SdkObject {
     this._deleted = true;
     if (!this._unaccessibleErrorMessage)
       await fs.promises.unlink(this._localPath).catch(e => {});
-    await this.reportFinished('File deleted upon browser context closure.');
+    await this.reportFinished(new TargetClosedError());
   }
 
-  async reportFinished(error?: string) {
+  async reportFinished(error?: Error) {
     if (this._finished)
       return;
     this._finished = true;
-    this._failureError = error || null;
+    this._failureError = error;
 
     if (error) {
       for (const callback of this._saveCallbacks)

packages/playwright-core/src/server/browser.ts

@@ -119,7 +119,7 @@ export abstract class Browser extends SdkObject {
     const download = this._downloads.get(uuid);
     if (!download)
       return;
-    download.artifact.reportFinished(error);
+    download.artifact.reportFinished(error ? new Error(error) : undefined);
     this._downloads.delete(uuid);
   }
 

packages/playwright-core/src/server/chromium/crBrowser.ts

@@ -269,7 +269,7 @@ export class CRBrowser extends Browser {
     if (payload.state === 'completed')
       this._downloadFinished(payload.guid, '');
     if (payload.state === 'canceled')
-      this._downloadFinished(payload.guid, 'canceled');
+      this._downloadFinished(payload.guid, this._closeReason || 'canceled');
   }
 
   async _closePage(crPage: CRPage) {

packages/playwright-core/src/server/dispatchers/artifactDispatcher.ts

@@ -44,14 +44,14 @@ export class ArtifactDispatcher extends Dispatcher<Artifact, channels.ArtifactCh
 
   async pathAfterFinished(): Promise<channels.ArtifactPathAfterFinishedResult> {
     const path = await this._object.localPathAfterFinished();
-    return { value: path || undefined };
+    return { value: path };
   }
 
   async saveAs(params: channels.ArtifactSaveAsParams): Promise<channels.ArtifactSaveAsResult> {
     return await new Promise((resolve, reject) => {
       this._object.saveAs(async (localPath, error) => {
-        if (error !== undefined) {
+        if (error) {
-          reject(new Error(error));
+          reject(error);
           return;
         }
         try {
@@ -68,8 +68,8 @@ export class ArtifactDispatcher extends Dispatcher<Artifact, channels.ArtifactCh
   async saveAsStream(): Promise<channels.ArtifactSaveAsStreamResult> {
     return await new Promise((resolve, reject) => {
       this._object.saveAs(async (localPath, error) => {
-        if (error !== undefined) {
+        if (error) {
-          reject(new Error(error));
+          reject(error);
           return;
         }
         try {
@@ -92,8 +92,6 @@ export class ArtifactDispatcher extends Dispatcher<Artifact, channels.ArtifactCh
 
   async stream(): Promise<channels.ArtifactStreamResult> {
     const fileName = await this._object.localPathAfterFinished();
-    if (!fileName)
-      return {};
     const readable = fs.createReadStream(fileName, { highWaterMark: 1024 * 1024 });
     return { stream: new StreamDispatcher(this, readable) };
   }

packages/playwright-core/src/server/firefox/ffBrowser.ts

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import { kTargetClosedErrorMessage } from '../../common/errors';
+import { TargetClosedError } from '../../common/errors';
 import { assert } from '../../utils';
 import type { BrowserOptions } from '../browser';
 import { Browser } from '../browser';
@@ -159,7 +159,7 @@ export class FFBrowser extends Browser {
 
   _onDisconnect() {
     for (const video of this._idToVideo.values())
-      video.artifact.reportFinished(kTargetClosedErrorMessage);
+      video.artifact.reportFinished(new TargetClosedError());
     this._idToVideo.clear();
     for (const ffPage of this._ffPages.values())
       ffPage.didClose();

packages/playwright-core/src/server/webkit/wkBrowser.ts

@@ -30,7 +30,7 @@ import type { Protocol } from './protocol';
 import type { PageProxyMessageReceivedPayload } from './wkConnection';
 import { kPageProxyMessageReceived, WKConnection, WKSession } from './wkConnection';
 import { WKPage } from './wkPage';
-import { kTargetClosedErrorMessage } from '../../common/errors';
+import { TargetClosedError } from '../../common/errors';
 import type { SdkObject } from '../instrumentation';
 
 const DEFAULT_USER_AGENT = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.4 Safari/605.1.15';
@@ -81,7 +81,7 @@ export class WKBrowser extends Browser {
       wkPage.didClose();
     this._wkPages.clear();
     for (const video of this._idToVideo.values())
-      video.artifact.reportFinished(kTargetClosedErrorMessage);
+      video.artifact.reportFinished(new TargetClosedError());
     this._idToVideo.clear();
     this._didClose();
   }

packages/playwright-core/types/types.d.ts

@@ -16897,7 +16897,7 @@ export interface Download {
   /**
    * Returns readable stream for current download or `null` if download failed.
    */
-  createReadStream(): Promise<null|Readable>;
+  createReadStream(): Promise<Readable>;
 
   /**
    * Deletes the downloaded file. Will wait for the download to finish if necessary.
@@ -16922,7 +16922,7 @@ export interface Download {
    * [download.suggestedFilename()](https://playwright.dev/docs/api/class-download#download-suggested-filename) to get
    * suggested file name.
    */
-  path(): Promise<null|string>;
+  path(): Promise<string>;
 
   /**
    * Copy the download to a user-specified path. It is safe to call this method while the download is still in progress.

packages/protocol/src/channels.ts

@@ -3862,7 +3862,7 @@ export interface ArtifactChannel extends ArtifactEventTarget, Channel {
 export type ArtifactPathAfterFinishedParams = {};
 export type ArtifactPathAfterFinishedOptions = {};
 export type ArtifactPathAfterFinishedResult = {
-  value?: string,
+  value: string,
 };
 export type ArtifactSaveAsParams = {
   path: string,
@@ -3884,7 +3884,7 @@ export type ArtifactFailureResult = {
 export type ArtifactStreamParams = {};
 export type ArtifactStreamOptions = {};
 export type ArtifactStreamResult = {
-  stream?: StreamChannel,
+  stream: StreamChannel,
 };
 export type ArtifactCancelParams = {};
 export type ArtifactCancelOptions = {};

packages/protocol/src/protocol.yml

@@ -3056,7 +3056,7 @@ Artifact:
 
     pathAfterFinished:
       returns:
-        value: string?
+        value: string
 
     # Blocks path/failure/delete/context.close until saved to the local |path|.
     saveAs:
@@ -3074,7 +3074,7 @@ Artifact:
 
     stream:
       returns:
-        stream: Stream?
+        stream: Stream
 
     cancel:
 

tests/library/download.spec.ts

@@ -19,6 +19,7 @@ import fs from 'fs';
 import path from 'path';
 import crypto from 'crypto';
 import type { Download } from 'playwright-core';
+import { kTargetClosedErrorMessage } from '../config/errors';
 
 it.describe('download event', () => {
   it.skip(({ mode }) => mode !== 'default', 'download.path() is not available in remote mode');
@@ -115,7 +116,7 @@ it.describe('download event', () => {
     expect(download.suggestedFilename()).toBe(`file.txt`);
     await download.path().catch(e => error = e);
     expect(await download.failure()).toContain('acceptDownloads');
-    expect(error.message).toContain('acceptDownloads: true');
+    expect(error!.message).toContain('acceptDownloads: true');
     await page.close();
   });
 
@@ -421,12 +422,12 @@ it.describe('download event', () => {
       // probably because of http -> https link.
       page.click('a', { modifiers: ['Alt'] })
     ]);
-    const [downloadPath, saveError] = await Promise.all([
+    const [downloadError, saveError] = await Promise.all([
-      download.path(),
+      download.path().catch(e => e),
       download.saveAs(testInfo.outputPath('download.txt')).catch(e => e),
       page.context().close(),
     ]);
-    expect(downloadPath).toBe(null);
+    expect(downloadError.message).toBe('download.path: canceled');
     expect([
       'download.saveAs: File not found on disk. Check download.failure() for details.',
       'download.saveAs: canceled',
@@ -451,17 +452,20 @@ it.describe('download event', () => {
       page.waitForEvent('download'),
       page.click('a')
     ]);
-    const [downloadPath, saveError] = await Promise.all([
+    const [downloadError, saveError] = await Promise.all([
-      download.path(),
+      download.path().catch(e => e),
       download.saveAs(testInfo.outputPath('download.txt')).catch(e => e),
       page.context().close(),
     ]);
-    expect(downloadPath).toBe(null);
     // The exact error message is racy, because sometimes browser is fast enough
     // to cancel the download.
+    expect([
+      'download.path: canceled',
+      'download.path: ' + kTargetClosedErrorMessage,
+    ]).toContain(downloadError.message);
     expect([
       'download.saveAs: canceled',
-      'download.saveAs: File deleted upon browser context closure.',
+      'download.saveAs: ' + kTargetClosedErrorMessage,
     ]).toContain(saveError.message);
   });
 
@@ -482,13 +486,13 @@ it.describe('download event', () => {
       page.waitForEvent('download'),
       page.click('a')
     ]);
-    const [downloadPath, saveError] = await Promise.all([
+    const [downloadError, saveError] = await Promise.all([
-      download.path(),
+      download.path().catch(e => e),
       download.saveAs(testInfo.outputPath('download.txt')).catch(e => e),
       (browser as any)._channel.killForTests(),
     ]);
-    expect(downloadPath).toBe(null);
+    expect(downloadError.message).toBe('download.path: ' + kTargetClosedErrorMessage);
-    expect(saveError.message).toContain('File deleted upon browser context closure.');
+    expect(saveError.message).toContain('download.saveAs: ' + kTargetClosedErrorMessage);
     await browser.close();
   });
 
@@ -512,10 +516,10 @@ it.describe('download event', () => {
 
     const stream = await download.createReadStream();
     const data = await new Promise<Buffer>((fulfill, reject) => {
-      const bufs = [];
+      const buffs: Buffer[] = [];
-      stream.on('data', d => bufs.push(d));
+      stream.on('data', d => buffs.push(d));
       stream.on('error', reject);
-      stream.on('end', () => fulfill(Buffer.concat(bufs)));
+      stream.on('end', () => fulfill(Buffer.concat(buffs)));
     });
     expect(data.byteLength).toBe(content.byteLength);
     expect(data.equals(content)).toBe(true);
@@ -585,7 +589,7 @@ it.describe('download event', () => {
       page.waitForEvent('download'),
       page.frame({
         url: server.PREFIX + '/3'
-      }).click('text=download')
+      })!.click('text=download')
     ]);
     const userPath = testInfo.outputPath('download.txt');
     await download.saveAs(userPath);
@@ -733,10 +737,10 @@ async function assertDownloadToPDF(download: Download, filePath: string) {
   expect(download.suggestedFilename()).toBe(path.basename(filePath));
   const stream = await download.createReadStream();
   const data = await new Promise<Buffer>((fulfill, reject) => {
-    const bufs = [];
+    const buffs: Buffer[] = [];
-    stream.on('data', d => bufs.push(d));
+    stream.on('data', d => buffs.push(d));
     stream.on('error', reject);
-    stream.on('end', () => fulfill(Buffer.concat(bufs)));
+    stream.on('end', () => fulfill(Buffer.concat(buffs)));
   });
   expect(download.url().endsWith('/' + path.basename(filePath))).toBeTruthy();
   const expectedPrefix = '%PDF';