-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[filesystem] improve download and allow downloading of big files and parallel downloads. #4890
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful feature, looks really robust!
I tried a few things despite the link not being copied on Linux, and it works great! Single or multiple files, it archives it with what looks like the closest common parent, works in a multi-root workspace as well!
Although in a multi-root workspace, I tried downloading files from two relatively different places, and I ended up with an archive containing 2 archives. It works, but I wonder if this is intended.
Anyway, last but not least, maybe I missed it, but is there a way to list the downloads?
Either a dedicated view with the different links, or maybe just a quick-pick menu that would open the links once we select an item (since we are having troubles with the copy
command)
IIUC you do track downloads already: https://github.com/theia-ide/theia/blob/91943cf34eb8837fb631d7c6119d499da07c6d94/packages/filesystem/src/node/download/file-download-collection.ts#L28-L89
I like these changes.
packages/filesystem/src/browser/download/file-download-service.ts
Outdated
Show resolved
Hide resolved
Regarding the offline, I've experienced this as well on gitpod, but i have not experienced it on my own server. This is probably something of a server limitation? I'm not 100% sure. And about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i have not experienced it on my own server.
It happened with my local setup.
And about the
unable to download file
, the links actually expire after 1 minute.
It wasn't related to timeout or something. When I select two nodes from the navigator, I select Copy Download Link and I paste the link to the browser, I got the error.
packages/filesystem/src/browser/download/file-download-service.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser/download/file-download-service.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser/download/file-download-service.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/browser/download/file-download-service.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/node/download/file-download-collection.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried it in Chrome, it works nicely. Thank you for your contributions, @uniibu!
Please, see my comments.
packages/filesystem/src/node/download/file-download-endpoint.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/node/download/file-download-collection.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/node/download/file-download-collection.ts
Outdated
Show resolved
Hide resolved
packages/filesystem/src/node/download/file-download-collection.ts
Outdated
Show resolved
Hide resolved
This PR improves the download by letting the native browser handle it instead of fetching it in the background. This also adds a unique link which expires in 1 minute. Tested on chrome 73.0 and firefox 60.5 Signed-off-by: Uni Sayo <unibtc@gmail.com>
@@ -40,8 +37,6 @@ export class FileDownloadService { | |||
@inject(FileSystem) | |||
protected readonly fileSystem: FileSystem; | |||
|
|||
@inject(StatusBar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the CI:
[lint] ERROR: /home/travis/build/theia-ide/theia/packages/filesystem/src/browser/download/file-download-service.ts:40:1 - Consecutive blank lines are forbidden
protected anchor: HTMLAnchorElement | undefined; | ||
protected downloadQueue: number[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All symbols with protected
and public
(default) visibility are part of the API, if you change it, you have to declare your modifications as a breaking change in the changelog.
@@ -147,54 +142,68 @@ export class FileDownloadService { | |||
return this.deferredUpload.promise; | |||
} | |||
|
|||
protected handleCopy(event: ClipboardEvent, downloadUrl: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All methods and functions with protected
and public
(default) visibility, must explicitly declare the return type. For instance, here, it is : void
.
Please, apply this rule to all the methods you have introduced.
@kittaakos @marechal-p Is it good? Can it be merged? |
I can see unresolved conversations, and the build is broken; probably it is not ready for the merge. |
Hi guys, sorry for the delay, been busy lately with work. Ill try to finish this week. Cheers |
@akosyakov Yes for sure. I've been caught up with work and couldn't literally find time right now. So if anyone wants to continue it, it's very much fine with me :) Sorry again guys. |
👍 We continue the work here: #5466 |
closing it in favor of #5466 |
Summary:
This PR improves downloading of files from Theia including:
Copy Download Link
menu to support the above.TLDR;
Currently, there are two main concerns regarding downloads. One, the client download is handled in the client background via fetch which stores the blob in the memory. There are limitations using this approach, one, blob can only handle a specific size depending on the browser and this is prone to OOM(Out of Memory) error. Two, the backend that serves the file uses
fs.readFile
which is not very sufficient specially for big files.To handle the first issue, instead of fetching the resource/file in the backend, the frontend instead makes an initial request to download a file, and the backend 'prepares' it and responds with a unique id to be used to download. The frontend then attaches this unique download link to the anchor tag with a
download
property and simulates a click. The download is now handled natively by the browser.For the second issue,the simplest solution was to use a readable stream piped to the response so that the backend does not need to store the whole file in memory before serving it to the user.
Improvements, since this pr now allows downloading big project files/folders, some of us prefer to use a separate downloader which downloads the files in parallel. So I added support for parallel downloads using
Range
header. And lastly, to be able to use a separate downloader, you will need the download link, so I added aCopy Download Link
menu option which works for singe and multiple(tar) files/folders. Cheers!Signed-off-by: Uni Sayo unibtc@gmail.com