From fdf130ace1bac50f7ef75a8187ed8bf62b0190a9 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Mon, 10 Jun 2024 21:48:51 +0200 Subject: [PATCH] feedback 4 --- .../src/client/elementHandle.ts | 30 +++++++++++-------- .../playwright-core/src/protocol/validator.ts | 3 ++ .../dispatchers/browserContextDispatcher.ts | 3 +- .../dispatchers/writableStreamDispatcher.ts | 24 ++++++++------- packages/playwright-core/src/server/dom.ts | 22 +++++++------- .../src/server/fileUploadUtils.ts | 11 +++---- packages/protocol/src/channels.ts | 5 ++++ packages/protocol/src/protocol.yml | 3 ++ 8 files changed, 58 insertions(+), 43 deletions(-) diff --git a/packages/playwright-core/src/client/elementHandle.ts b/packages/playwright-core/src/client/elementHandle.ts index 7641e5cb68c43..deb335e3b0f1d 100644 --- a/packages/playwright-core/src/client/elementHandle.ts +++ b/packages/playwright-core/src/client/elementHandle.ts @@ -250,7 +250,7 @@ export function convertSelectOptionValues(values: string | api.ElementHandle | S return { options: values as SelectOption[] }; } -type SetInputFilesFiles = Pick; +type SetInputFilesFiles = Pick; function filePayloadExceedsSizeLimit(payloads: FilePayload[]) { return payloads.reduce((size, item) => size + (item.buffer ? item.buffer.byteLength : 0), 0) >= fileUploadSizeLimit; @@ -262,33 +262,37 @@ export async function convertInputFiles(files: string | FilePayload | string[] | if (items.some(item => typeof item === 'string')) { if (!items.every(item => typeof item === 'string')) throw new Error('File paths cannot be mixed with buffers'); - const itemFileTypes = (await Promise.all(items.map(async item => (await fs.promises.stat(item as string)).isDirectory() ? 'directory' : 'file'))); + const itemFileTypes = (await Promise.all((items as string[]).map(async item => (await fs.promises.stat(item)).isDirectory() ? 'directory' : 'file'))); if (new Set(itemFileTypes).size > 1 || itemFileTypes.filter(type => type === 'directory').length > 1) throw new Error('File paths must be all files or a single directory'); if (context._connection.isRemote()) { - const streams = (await Promise.all((items as string[]).map(async item => { - const isDirectory = (await fs.promises.stat(item)).isDirectory(); - const files = isDirectory ? (await fs.promises.readdir(item, { withFileTypes: true, recursive: true })).filter(f => f.isFile()).map(f => path.join(f.path, f.name)) : [item]; - const { writableStreams } = await context._wrapApiCall(async () => context._channel.createTempFiles({ - rootDirName: isDirectory ? path.basename(item) : undefined, - items: await Promise.all(files.map(async file => { - const lastModifiedMs = (await fs.promises.stat(file)).mtimeMs; + const streams = (await Promise.all((items).map(async item => { + const isDirectory = (await fs.promises.stat(item as string)).isDirectory(); + const files = isDirectory ? (await fs.promises.readdir(item as string, { withFileTypes: true, recursive: true })).filter(f => f.isFile()).map(f => path.join(f.path, f.name)) : [item]; + const { writableStreams, rootDir } = await context._wrapApiCall(async () => context._channel.createTempFiles({ + rootDirName: isDirectory ? path.basename(item as string) : undefined, + items: await Promise.all((files as string[]).map(async file => { + const lastModifiedMs = (await fs.promises.stat(item as string)).mtimeMs; return { - name: isDirectory ? path.relative(item, file) : path.basename(file), + name: isDirectory ? path.relative(item as string, file) : path.basename(file), lastModifiedMs }; })), }), true); for (let i = 0; i < files.length; i++) { const writable = WritableStream.from(writableStreams[i]); - await pipelineAsync(fs.createReadStream(files[i]), writable.stream()); + await pipelineAsync(fs.createReadStream((files as string[])[i]), writable.stream()); } - return writableStreams; + return rootDir ?? writableStreams; }))).flat(); return { streams }; } - return { localPaths: items.map(f => path.resolve(f as string)) as string[] }; + const resolvedItems = (items as string[]).map(f => path.resolve(f)); + return { + localPaths: resolvedItems.filter((_, i) => itemFileTypes[i] === 'file'), + localDirectory: resolvedItems.find((_, i) => itemFileTypes[i] === 'directory'), + }; } const payloads = items as FilePayload[]; diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index ef0d85c5f2ed1..f976a0583460f 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -959,6 +959,7 @@ scheme.BrowserContextCreateTempFilesParams = tObject({ })), }); scheme.BrowserContextCreateTempFilesResult = tObject({ + rootDir: tOptional(tChannel(['WritableStream'])), writableStreams: tArray(tChannel(['WritableStream'])), }); scheme.BrowserContextUpdateSubscriptionParams = tObject({ @@ -1629,6 +1630,7 @@ scheme.FrameSetInputFilesParams = tObject({ mimeType: tOptional(tString), buffer: tBinary, }))), + localDirectory: tOptional(tString), localPaths: tOptional(tArray(tString)), streams: tOptional(tArray(tChannel(['WritableStream']))), timeout: tOptional(tNumber), @@ -1996,6 +1998,7 @@ scheme.ElementHandleSetInputFilesParams = tObject({ mimeType: tOptional(tString), buffer: tBinary, }))), + localDirectory: tOptional(tString), localPaths: tOptional(tArray(tString)), streams: tOptional(tArray(tChannel(['WritableStream']))), timeout: tOptional(tNumber), diff --git a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts index c4a23fda250f5..c9c952ed1e283 100644 --- a/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserContextDispatcher.ts @@ -185,10 +185,11 @@ export class BrowserContextDispatcher extends Dispatcher { await fs.promises.mkdir(path.dirname(path.join(tempDirWithRootName, item.name)), { recursive: true }); const file = fs.createWriteStream(path.join(tempDirWithRootName, item.name)); - return new WritableStreamDispatcher(this, file, item.lastModifiedMs, params.rootDirName ? tempDirWithRootName : undefined); + return new WritableStreamDispatcher(this, file, item.lastModifiedMs); })) }; } diff --git a/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts b/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts index f2ab6400365d1..86623fbfcf653 100644 --- a/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/writableStreamDispatcher.ts @@ -20,19 +20,19 @@ import * as fs from 'fs'; import { createGuid } from '../../utils'; import type { BrowserContextDispatcher } from './browserContextDispatcher'; -export class WritableStreamDispatcher extends Dispatcher<{ guid: string, stream: fs.WriteStream }, channels.WritableStreamChannel, BrowserContextDispatcher> implements channels.WritableStreamChannel { +export class WritableStreamDispatcher extends Dispatcher<{ guid: string, streamOrDirectory: fs.WriteStream | string }, channels.WritableStreamChannel, BrowserContextDispatcher> implements channels.WritableStreamChannel { _type_WritableStream = true; private _lastModifiedMs: number | undefined; - private _rootDir: string | undefined; - constructor(scope: BrowserContextDispatcher, stream: fs.WriteStream, lastModifiedMs?: number, rootDir?: string) { - super(scope, { guid: 'writableStream@' + createGuid(), stream }, 'WritableStream', {}); + constructor(scope: BrowserContextDispatcher, streamOrDirectory: fs.WriteStream | string, lastModifiedMs?: number) { + super(scope, { guid: 'writableStream@' + createGuid(), streamOrDirectory }, 'WritableStream', {}); this._lastModifiedMs = lastModifiedMs; - this._rootDir = rootDir; } async write(params: channels.WritableStreamWriteParams): Promise { - const stream = this._object.stream; + if (typeof this._object.streamOrDirectory === 'string') + throw new Error('Cannot write to a directory'); + const stream = this._object.streamOrDirectory; await new Promise((fulfill, reject) => { stream.write(params.binary, error => { if (error) @@ -44,17 +44,21 @@ export class WritableStreamDispatcher extends Dispatcher<{ guid: string, stream: } async close() { - const stream = this._object.stream; + if (typeof this._object.streamOrDirectory === 'string') + throw new Error('Cannot close a directory'); + const stream = this._object.streamOrDirectory; await new Promise(fulfill => stream.end(fulfill)); if (this._lastModifiedMs) await fs.promises.utimes(this.path(), new Date(this._lastModifiedMs), new Date(this._lastModifiedMs)); } path(): string { - return this._object.stream.path as string; + if (typeof this._object.streamOrDirectory === 'string') + return this._object.streamOrDirectory; + return this._object.streamOrDirectory.path as string; } - rootDir(): string | undefined { - return this._rootDir; + isDirectory(): boolean { + return typeof this._object.streamOrDirectory === 'string'; } } diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index 86577528f1fc3..ecf6c8f92c8e3 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -34,6 +34,7 @@ import { prepareFilesForUpload } from './fileUploadUtils'; export type InputFilesItems = { filePayloads?: types.FilePayload[], localPaths?: string[] + localDirectory?: string }; type ActionName = 'click' | 'hover' | 'dblclick' | 'tap' | 'move and up' | 'move and down'; @@ -625,13 +626,9 @@ export class ElementHandle extends js.JSHandle { } async _setInputFiles(progress: Progress, items: InputFilesItems, options: types.NavigatingActionWaitOptions): Promise<'error:notconnected' | 'done'> { - const { filePayloads, localPaths } = items; - const localPathsFileTypes = (await Promise.all((localPaths ?? []).map(async item => (await fs.promises.stat(item as string)).isDirectory() ? 'directory' : 'file'))); - if (new Set(localPathsFileTypes).size > 1 || localPathsFileTypes.filter(type => type === 'directory').length > 1) - throw new Error('File paths must be all files or a single directory'); - const doesLocalPathsIncludeDirectory = localPathsFileTypes.includes('directory'); + const { filePayloads, localPaths, localDirectory } = items; const multiple = filePayloads && filePayloads.length > 1 || localPaths && localPaths.length > 1; - const result = await this.evaluateHandleInUtility(([injected, node, { multiple, doesLocalPathsIncludeDirectory }]): Element | undefined => { + const result = await this.evaluateHandleInUtility(([injected, node, { multiple, directoryUpload }]): Element | undefined => { const element = injected.retarget(node, 'follow-label'); if (!element) return; @@ -640,24 +637,25 @@ export class ElementHandle extends js.JSHandle { const inputElement = element as HTMLInputElement; if (multiple && !inputElement.multiple && !inputElement.webkitdirectory) throw injected.createStacklessError('Non-multiple file input can only accept single file'); - if (doesLocalPathsIncludeDirectory && !inputElement.webkitdirectory) + if (directoryUpload && !inputElement.webkitdirectory) throw injected.createStacklessError('File input does not support directories, pass individual files instead'); return inputElement; - }, { multiple, doesLocalPathsIncludeDirectory }); + }, { multiple, directoryUpload: !!localDirectory }); if (result === 'error:notconnected' || !result.asElement()) return 'error:notconnected'; const retargeted = result.asElement() as ElementHandle; await progress.beforeInputAction(this); await this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { progress.throwIfAborted(); // Avoid action that has side-effects. - if (localPaths) { - await Promise.all(localPaths.map(localPath => ( + if (localPaths || localDirectory) { + const localPathsOrDirectory = localDirectory ? [localDirectory] : localPaths!; + await Promise.all((localPathsOrDirectory).map(localPath => ( fs.promises.access(localPath, fs.constants.F_OK) ))); - const waitForInputEvent = doesLocalPathsIncludeDirectory ? this.evaluate(node => new Promise(fulfill => { + const waitForInputEvent = localDirectory ? this.evaluate(node => new Promise(fulfill => { node.addEventListener('input', fulfill, { once: true }); })).catch(() => {}) : Promise.resolve(); - await this._page._delegate.setInputFilePaths(retargeted, localPaths); + await this._page._delegate.setInputFilePaths(retargeted, localPathsOrDirectory); await waitForInputEvent; } else { await this._page._delegate.setInputFiles(retargeted, filePayloads!); diff --git a/packages/playwright-core/src/server/fileUploadUtils.ts b/packages/playwright-core/src/server/fileUploadUtils.ts index 402fe2fcd61f2..5e935057cd02f 100644 --- a/packages/playwright-core/src/server/fileUploadUtils.ts +++ b/packages/playwright-core/src/server/fileUploadUtils.ts @@ -31,17 +31,14 @@ async function filesExceedUploadLimit(files: string[]) { export async function prepareFilesForUpload(frame: Frame, params: channels.ElementHandleSetInputFilesParams): Promise { const { payloads, streams } = params; - let { localPaths } = params; + let { localPaths, localDirectory } = params; if ([payloads, localPaths, streams].filter(Boolean).length !== 1) throw new Error('Exactly one of payloads, localPaths and streams must be provided'); if (streams) { - const directoryMode = streams.every(c => (c as WritableStreamDispatcher).rootDir()); - if (directoryMode) - localPaths = Array.from(new Set(streams.map(c => (c as WritableStreamDispatcher).rootDir()!))); - else - localPaths = streams.map(c => (c as WritableStreamDispatcher).path()); + localPaths = streams.map(c => (c as WritableStreamDispatcher)).filter(s => !s.isDirectory()).map(s => s.path()); + localDirectory = streams.map(c => (c as WritableStreamDispatcher)).filter(s => s.isDirectory()).map(s => s.path())[0]; } if (localPaths) { for (const p of localPaths) @@ -78,5 +75,5 @@ export async function prepareFilesForUpload(frame: Frame, params: channels.Eleme lastModifiedMs: payload.lastModifiedMs })); - return { localPaths, filePayloads }; + return { localPaths, localDirectory, filePayloads }; } \ No newline at end of file diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 2f6dc63531589..8ed7b1ba5a3a1 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -1748,6 +1748,7 @@ export type BrowserContextCreateTempFilesOptions = { rootDirName?: string, }; export type BrowserContextCreateTempFilesResult = { + rootDir?: WritableStreamChannel, writableStreams: WritableStreamChannel[], }; export type BrowserContextUpdateSubscriptionParams = { @@ -2916,6 +2917,7 @@ export type FrameSetInputFilesParams = { mimeType?: string, buffer: Binary, }[], + localDirectory?: string, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, @@ -2928,6 +2930,7 @@ export type FrameSetInputFilesOptions = { mimeType?: string, buffer: Binary, }[], + localDirectory?: string, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, @@ -3540,6 +3543,7 @@ export type ElementHandleSetInputFilesParams = { mimeType?: string, buffer: Binary, }[], + localDirectory?: string, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, @@ -3551,6 +3555,7 @@ export type ElementHandleSetInputFilesOptions = { mimeType?: string, buffer: Binary, }[], + localDirectory?: string, localPaths?: string[], streams?: WritableStreamChannel[], timeout?: number, diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index db0afbb9d7ebe..096f88ec5ca65 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -1195,6 +1195,7 @@ BrowserContext: name: string lastModifiedMs: number? returns: + rootDir: WritableStream? writableStreams: type: array items: WritableStream @@ -2195,6 +2196,7 @@ Frame: name: string mimeType: string? buffer: binary + localDirectory: string? localPaths: type: array? items: string @@ -2755,6 +2757,7 @@ ElementHandle: name: string mimeType: string? buffer: binary + localDirectory: string? localPaths: type: array? items: string