Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create FileReadableStream class to stream large files for d&d uploads and add progress notification #98248

Closed
wants to merge 4 commits into from

Conversation

steven166
Copy link
Contributor

This PR fixes #83565

@isidorn
Copy link
Contributor

isidorn commented May 20, 2020

@steven166 great work both on this PR and the previos one. I plan to review this after a meeting today.
Would you maybe be interested in tackling this one as well? I planned to look into this next week and if you are interested we can look into it together. Let me know what you think. #93599

Thanks!

@bpasero
Copy link
Member

bpasero commented May 20, 2020

@steven166 thanks for bringing this back so quickly, can we keep the streams support but leave out notifications for now? I think the progress reporting via the progress bar in the explorer is OK for now. We can always revisit that later, but I think notifications are a bit too much for my taste when it comes to uploading. There are other more lightweight ways to report progress, e.g. in the status bar too.

@steven166
Copy link
Contributor Author

@isidorn yeah sure, I can help with that one.

@bpasero I've disabled the notification part for now. I think it can be a long running operation (like minutes) where the user is waiting for, so that is why I choose for a notification. But I'm not sure when to use a notification or the status bar.

@bpasero
Copy link
Member

bpasero commented May 27, 2020

@steven166 I think I found a much simpler solution by leveraging the file.stream property. Instead of doing this all on our own, we can simply get a stream from the file object. As such, the upload method would just be this:

private async doUploadWebFileEntry(entry: IWebkitDataTransferItemEntry, parentResource: URI, target: ExplorerItem | undefined): Promise<{ isFile: boolean, resource: URI } | undefined> {
	if (!entry.name || !entry.isFile && !entry.isDirectory) {
		return undefined;
	}

	const resource = joinPath(parentResource, entry.name);

	// Confirm overwrite as needed
	if (target && target.getChild(entry.name)) {
		const { confirmed } = await this.dialogService.confirm(getFileOverwriteConfirm(resource.path));
		if (!confirmed) {
			return undefined;
		}
	}

	// Handle file upload
	if (entry.isFile) {
		const writeableStream = newWriteableBufferStream();

		// Read the file in chunks using File.stream() web APIs
		(async () => {
			try {
				const file = await new Promise<File>((resolve, reject) => entry.file(resolve, reject));
				const reader: ReadableStreamDefaultReader<Uint8Array> = file.stream().getReader();

				let res = await reader.read();
				while (!res.done) {
					writeableStream.write(VSBuffer.wrap(res.value));

					res = await reader.read();
				}
				writeableStream.end(res.value ? VSBuffer.wrap(res.value) : undefined);
			} catch (error) {
				writeableStream.end(error);
			}
		})();

		await this.fileService.writeFile(resource, writeableStream);
		return { isFile: true, resource };
	}

	// Handle folder upload
	else {

		// Create target folder
		await this.fileService.createFolder(resource);

		// Recursive upload files in this directory
		const folderTarget = target && target.getChild(entry.name) || undefined;
		const dirReader = entry.createReader();
		const childEntries = await new Promise<IWebkitDataTransferItemEntry[]>((resolve, reject) => {
			dirReader.readEntries(resolve, reject);
		});

		for (let childEntry of childEntries) {
			await this.doUploadWebFileEntry(childEntry, resource, folderTarget);
		}

		return { isFile: false, resource };
	}
}

Seems to work great, except for Safari where file.stream seems undefined.

@bpasero
Copy link
Member

bpasero commented May 29, 2020

I landed this as 203508d with some minimal progress. Thanks for this PR that made me look into the file.stream API, but I prefer that approach.

@bpasero bpasero closed this May 29, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web: improve the experience for large file uploads
3 participants