feat(downloads): support downloads on cr and wk #1632

pavelfeldman · 2020-04-02T00:41:16Z

No description provided.

JoelEinbinder · 2020-04-02T17:20:25Z

docs/api.md

+Deletes the downloaded file.
+
+#### download.error()
+- returns: <[Promise]<null|[string]>>


What kind of errors are returned here? When should I use this string?

WebKit will report network error here. If you did not pass acceptDownloads, you get canceled error as well.

Can this match the response.failure() api? It is equivalent right?

It is not equivalent, but it is similar, so i can rename.

yury-s · 2020-04-02T17:08:21Z

src/browser.ts

-  const page = await context.newPage();
-  page._ownedContext = context;
-  return page;
+export abstract class BrowserBase extends EventEmitter implements Browser {


nit: Browser already extends EventEmitter

My understanding is that Browser is an interface, so it can't extend class

yury-s · 2020-04-02T17:13:48Z

src/chromium/crBrowser.ts


  async _initialize() {
+    const promises: Promise<any>[] = [
+      this._browser._session.send('Browser.setDownloadBehavior', {


Shouldn't contextId be passed too?

yury-s · 2020-04-02T17:16:11Z

src/download.ts

+import { Readable } from 'stream';
+
+export class Download {
+  private _downloadsPath: string;


nit: readonly?

We don't do it systematically :/

yury-s · 2020-04-02T17:20:43Z

src/server/processLauncher.ts

 import { helper } from '../helper';
+import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';


nit: run 'Organize imports'?

yury-s · 2020-04-02T17:23:22Z

src/transport.ts

+
+export class InterceptingTransport implements ConnectionTransport {
+  private readonly _delegate: ConnectionTransport;
+  private _interceptor: (message: ProtocolRequest) => ProtocolRequest;


nit: readonly too?

yury-s · 2020-04-02T17:26:28Z

src/webkit/wkPage.ts

    this._page._frameManager.frameLifecycleEvent(frameId, event);
  }

+  private _onDownloadCreated() {


yury-s · 2020-04-02T17:28:38Z

test/download.spec.js

+      expect(fs.existsSync(path1)).toBeFalsy();
+      expect(fs.existsSync(path2)).toBeFalsy();
+    });
+    it.fail(true)('should delete downloads on browser gone', async ({ server, defaultBrowserOptions }) => {


Unskip now that Joel has a fix?

As soon as it lands

JoelEinbinder · 2020-04-02T17:30:55Z

src/browserContext.ts

  }

-  _didCloseInternal() {
+  async _didCloseInternal(omitDeleteDownloads = false) {


This was hard for me to follow. Can we leave it as _didCloseInternal(), and add async deleteDownloads(). Is it important that the close event fires before the downloads have been deleted?

My worry is that it is easy to forget to call deleteDownloads. As of now, they know they need to call _didCloseInternal. Or what do you mean?

src/chromium/crConnection.ts

JoelEinbinder · 2020-04-02T17:37:17Z

utils/doclint/check_public_api/index.js

      [/object/g, 'Object'],
-      [/Promise\<T\>/, 'Promise<Object>']
+      [/Promise\<T\>/, 'Promise<Object>'],
+      [/Readable/, 'stream.Readable']


If we leave this as Readable, it shouldn't need any special casing or overrides.

yury-s · 2020-04-02T17:39:15Z

src/download.ts

+    return fileName;
+  }
+
+  async error(): Promise<string | null> {


May be waitForFinished() ?

JoelEinbinder · 2020-04-02T17:39:21Z

test/playwright.spec.js

      state._stderr.on('line', dumperr);
      if (dumpProtocolOnFailure)
-        state.browser._setDebugFunction(data => test.output.push(`\x1b[32m[pw:protocol]\x1b[0m ${data}`));
+        debug('pw:protocol').output = data => test.output.push(`\x1b[32m[pw:protocol]\x1b[0m ${data}`);


in my console: debug('str') !== debug('str')
How does this work?

Good question.

dgozman · 2020-04-02T22:00:26Z

docs/api.md

+#### event: 'download'
+- <[Download]>
+
+Emitted when attachment is downloaded. User can access basic file operations on downloaded content via the passed [Download] instance. Browser context must be created with the `acceptDownloads` set to `true` when user needs access to the downloaded content. If `acceptDownloads` is not set or set to `false`, download events are emitted, but the actual download is not performed and user has no access to the downloaded files.


super nit: we don't usually mention "user" in the docs.

dgozman · 2020-04-02T22:10:22Z

src/download.ts

+  async path(): Promise<string | null> {
+    if (!this._acceptDownloads)
+      throw new Error('Pass { acceptDownloads: true } when you are creating your browser context.');
+    const fileName = path.join(this._downloadsPath, this._uuid);


Is there a way to retrieve suggested file name, extension, mime type or url? I imagine that would be very handy, e.g. to assert that my "Download" button tries to download the "screenshot-{current-time}.png" file.

Yes, that'd be super handy. Follow up?

dgozman · 2020-04-02T22:11:57Z

src/download.ts

+  async delete(): Promise<void> {
+    if (!this._acceptDownloads || this._deleted)
+      return;
+    const fileName = await this.path();


Should check _deleted after this await to avoid a race.

dgozman · 2020-04-02T22:15:25Z

src/download.ts

+      return;
+    const fileName = await this.path();
+    this._deleted = true;
+    if (fileName && await util.promisify(fs.exists)(fileName))


Asynchronous exists check is racy. Better to always call unlink and catch ENOENT error.

dgozman · 2020-04-02T22:20:24Z

test/download.spec.js

+      await browser.close();
+      expect(fs.existsSync(path1)).toBeFalsy();
+      expect(fs.existsSync(path2)).toBeFalsy();
+      console.log(path.join(path1, '..'));


dgozman · 2020-04-02T22:20:54Z

test/download.spec.js

+      expect(fs.existsSync(path.join(path1, '..'))).toBeFalsy();
+    });
+    it('make coverage happy', async({page}) => {
+      // Otherwise FF gets mad.


There is a list of not covered methods in playwright.spec.js.

dgozman · 2020-04-02T22:21:45Z

test/playwright.spec.js

            state._stderr.off('line', dumperr);
            if (dumpProtocolOnFailure)
-              state.browser._setDebugFunction(() => void 0);
+              debug('pw:protocol').output = data => console.log;


Not sure what this meant to be, but looks suspicious :)

dgozman · 2020-04-02T22:22:33Z

src/transport.ts

  }
 }
+
+export class InterceptingTransport implements ConnectionTransport {


Where is this one used?

Nowhere just yet, but looks generally useful?

pavelfeldman force-pushed the fe_download branch from 5a81f42 to dec9bb6 Compare April 2, 2020 01:22

JoelEinbinder reviewed Apr 2, 2020

View reviewed changes

yury-s approved these changes Apr 2, 2020

View reviewed changes

JoelEinbinder reviewed Apr 2, 2020

View reviewed changes

src/chromium/crConnection.ts Show resolved Hide resolved

JoelEinbinder reviewed Apr 2, 2020

View reviewed changes

yury-s approved these changes Apr 2, 2020

View reviewed changes

JoelEinbinder reviewed Apr 2, 2020

View reviewed changes

pavelfeldman force-pushed the fe_download branch from dec9bb6 to cc1666f Compare April 2, 2020 20:09

dgozman reviewed Apr 2, 2020

View reviewed changes

pavelfeldman force-pushed the fe_download branch from cc1666f to 7735a80 Compare April 3, 2020 00:22

feat(downloads): support downloads on cr and wk

f7f4de8

pavelfeldman force-pushed the fe_download branch from 7735a80 to f7f4de8 Compare April 3, 2020 00:32

pavelfeldman merged commit 75571e8 into microsoft:master Apr 3, 2020

pavelfeldman deleted the fe_download branch April 17, 2020 21:12

feat(downloads): support downloads on cr and wk #1632

feat(downloads): support downloads on cr and wk #1632

Uh oh!

Conversation

pavelfeldman commented Apr 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment