-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Switch from downloadjs to multi-download #174
Changes from 7 commits
6fed0b5
2494dea
60e9185
f8acd96
d9cbb0c
0e6f609
8b9e588
72de2fe
abe585a
9cd46a8
b11cee7
89ba327
6f5c806
993e47b
a6ae197
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
import { h, createRef } from 'preact'; | ||
import download from 'downloadjs'; | ||
import { route } from 'preact-router'; | ||
import { PureComponent, forwardRef, memo } from 'preact/compat'; | ||
import { ArrowLeft, CheckCircle, Home, Plus, Image, Film, Box, Music, File, Zap, Share2, Send } from 'preact-feather'; | ||
|
@@ -22,6 +21,35 @@ import roomsDispatch from '../../../reducers/rooms'; | |
|
||
import './FileTransfer.scss'; | ||
|
||
|
||
// adapted from https://github.com/sindresorhus/multi-download/blob/v4.0.0/index.js | ||
// to take File https://developer.mozilla.org/en-US/docs/Web/API/File | ||
const delay = milliseconds => new Promise(resolve => { | ||
setTimeout(resolve, milliseconds); | ||
}); | ||
const download = async (file) => { | ||
const a = document.createElement('a'); | ||
a.download = file.name; | ||
a.href = URL.createObjectURL(file); | ||
a.style.display = 'none'; | ||
document.body.append(a); | ||
a.click(); | ||
|
||
// Chrome requires the timeout | ||
await delay(100); | ||
a.remove(); | ||
}; | ||
const multiDownload = async (files) => { | ||
if (!files) { | ||
throw new Error('`files` required'); | ||
} | ||
|
||
for (const [index, file] of files.entries()) { | ||
await delay(index * 1000); // eslint-disable-line no-await-in-loop | ||
download(file); | ||
} | ||
} | ||
|
||
ddelange marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const CanvasUnwrapped = (props, ref) => { | ||
return <canvas ref={ref} {...props} />; | ||
}; | ||
|
@@ -292,17 +320,17 @@ class FileTransfer extends PureComponent { | |
isSelectorEnabled: false, | ||
}); | ||
}, | ||
onDone: (file, meta) => { | ||
onDone: (files) => { | ||
if (file !== undefined) { | ||
if (Array.isArray(file)) { | ||
file.forEach(file => { | ||
file.getBlob((err, blob) => download(blob, file.name)); | ||
}); | ||
multiDownload( | ||
// make regular File from webtorrent File https://github.com/webtorrent/webtorrent/blob/v2.4.14/lib/file.js#L7 | ||
files.map(file => new File([file.getBlob()], file.name, {type: file.type})) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw, it looks like this seems related to them switching to local storage in v2 as opposed to keeping complete torrents in memory in v1 ref webtorrent/webtorrent#86 (comment) edit: they now have Maybe blaze can even get rid of
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blaze currently uses Webtorrent v1, and we'll need to upgrade to v2. From what I can recall since the last time I had checked, there were a bunch of API changes associated with v2 that we'd also have to make in Blaze which I wasn't fond of picking up then. Would you be interested in picking this up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I'm also afraid it's a can of worms... the local storage feature might make it worthwhile though. I guess a 10GB file would now fill up 10GB of my swap right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. judging by the File API docs, I think using streamURL you can initiate a download before the torrent has finished downloading, and so download.js could theoretically be called upon as soon as the torrent is initiated. most importantly, that way the file doesn't need to go fully into memory in a Blob in order to send to downloads. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. file.getBlob and file.blob are functionally identical, it was simply renamed to match the W3C file API, rather than create our own structure, but getBlob also loaded the entire file into memory There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah just saw your cross comment, so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@ddelange isn't the local storage limited to like 10MBs? I wonder how it would manage files of much higher sizes in such a limited space There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in v2 the torrents directly download to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
for the sake of simplicity lets say sure, but if you want specifics, it actually writes to individual chunks via FSA's origin private file system, which functionally is no different from the likes of localStorage or IDB, except its a LOT faster, and allows for MUCH, MUCH, MUCH bigger file sizes, in extreme cases up to terabytes on chrome that's a bit different, because IF YOU CHOOSE TO specify a rootDir, it can emulate the file structure in a specified folder as already mentioned, on top of saving chunks as cache to OPFS that's the big difference for webtorrent v2, the storage and interfaces it provides:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
); | ||
} | ||
else { | ||
download(file, meta.name, meta.type); | ||
download(file); | ||
} | ||
} | ||
this.resetState(); | ||
}, | ||
ddelange marked this conversation as resolved.
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.
Can we use
downloadjs
for this? From what I can see, delay of 1s is something that is to be added for fixing the issue. The individual file download seems to use same process as downloadjsThere 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.
in addition to the 1s delay, to me it looks like this additional 100ms delay before removing the
a
again is not present in downloadjs. it looks like they immediately remove it after clicking: https://github.com/rndme/download/blob/09d6492f47ef18feca39c3d748960dce44f93a89/download.js#L116-L117maybe chrome introduced the need for this additional short delay somewhere in the last 8 years (the latest release of downloadjs)?
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'd say this is a nice elimination of some legacy/unmaintained code, in exchange for a
download
function that's only 10 lines without dependenciesThere 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.
Ah I see the timeout. Changes seems fine to me, I'll also test it on my end in couple of browsers to ensure that it doesn't break existing flows.
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.
OK! we might be able to reduce the 1000ms, I blamed it to a commit related to cross-origin URLs sindresorhus/multi-download@bf7580d#r144977818