Skip to content
New issue

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

#

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

Already on GitHub? # to your account

feat: use direct links to download all files #1894

Merged
merged 7 commits into from
Nov 22, 2022
Merged

feat: use direct links to download all files #1894

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
480b955
fix: use direct link to gateway to download files
hacdias Jan 16, 2022
b8ec0cf
feat: massively simplify file downloading
hacdias Nov 10, 2022
d24c948
fix: download filename
hacdias Nov 10, 2022
6bbb524
refactor: remove download progress things
hacdias Nov 10, 2022
1f72e4b
refactor: remove return await
hacdias Nov 11, 2022
194ac18
refactor: use window.location.href instead
hacdias Nov 11, 2022
9bec6b6
Merge branch 'main' into fix/1887
SgtPooki Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/bundles/files/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,9 +396,8 @@ const actions = () => ({
* @param {FileStat[]} files
*/
doFilesDownloadLink: (files) => perform(ACTIONS.DOWNLOAD_LINK, async (ipfs, { store }) => {
const apiUrl = store.selectApiUrl()
const gatewayUrl = store.selectGatewayUrl()
return await getDownloadLink(files, gatewayUrl, apiUrl, ipfs)
return await getDownloadLink(files, gatewayUrl, ipfs)
}),

/**
Expand Down
27 changes: 4 additions & 23 deletions src/files/FilesPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { withTranslation, Trans } from 'react-i18next'
import ReactJoyride from 'react-joyride'
// Lib
import { filesTour } from '../lib/tours'
import downloadFile from './download-file'
// Components
import ContextMenu from './context-menu/ContextMenu'
import withTour from '../components/tour/withTour'
Expand All @@ -27,8 +26,6 @@ const FilesPage = ({
files, filesPathInfo, pinningServices, toursEnabled, handleJoyrideCallback, isCliTutorModeEnabled, cliOptions, t
}) => {
const contextMenuRef = useRef()
const [downloadAbort, setDownloadAbort] = useState(null)
const [downloadProgress, setDownloadProgress] = useState(null)
const [modals, setModals] = useState({ show: null, files: null })
const [contextMenu, setContextMenu] = useState({
isOpen: false,
Expand Down Expand Up @@ -58,28 +55,13 @@ const FilesPage = ({
*/

const onDownload = async (files) => {
if (downloadProgress !== null) {
return downloadAbort()
}

const { url, filename, method } = await doFilesDownloadLink(files)

if (method === 'GET') {
const link = document.createElement('a')
link.href = url
link.click()
} else {
const updater = (v) => setDownloadProgress(v)
const { abort } = await downloadFile(url, filename, updater, method)
setDownloadAbort(() => abort)
}
const url = await doFilesDownloadLink(files)
const link = document.createElement('a')
link.href = url
link.click()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking Q: instead of creating an element and then programmatically clicking on it can we have the thing the users click have the appropriate URL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I do agree that this is a significant improvement, and resolves #1887 but I'm wondering if there is a way we can keep the ability to cancel a download, or display download progress. Fine with a separate issue for those items

Downloads will be handled by the browser UI. The user can just cancel it through the browser UI. We only had the option to cancel and display progress on the Web UI itself because we were storing every single byte of data in-memory before downloading.

Copy link
Member Author

@hacdias hacdias Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking Q: instead of creating an element and then programmatically clicking on it can we have the thing the users click have the appropriate URL?

Yes and no. If you select multiple files, what link do you put? You can't as we calculate the CID dynamically. Then we'd need different behaviour according to the number of files selected and that's a bit of a hassle. Let's keep it like this to be uniform with the CAR downloading behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just redirect the users to download? it will prompt the user to download and not go away from current page too?

Copy link
Member Author

@hacdias hacdias Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whizzzkid I changed to window.location.href = url. However, it has the same behaviour in my browser as clicking a link. Because clicking a link just leads to redirection. I'm interested in knowing what is happening in your browser, because I can't understand. I'll post a video here in a bit (see #1894 (comment)).

}

const onDownloadCar = async (files) => {
if (downloadProgress !== null) {
return downloadAbort()
}

const url = await doFilesDownloadCarLink(files)
const link = document.createElement('a')
link.href = url
Expand Down Expand Up @@ -167,7 +149,6 @@ const FilesPage = ({
pendingPins={pendingPins}
failedPins={failedPins}
upperDir={files.upper}
downloadProgress={downloadProgress}
onShare={(files) => showModal(SHARE, files)}
onRename={(files) => showModal(RENAME, files)}
onRemove={(files) => showModal(DELETE, files)}
Expand Down
44 changes: 0 additions & 44 deletions src/files/download-file.js
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions src/files/files-list/FilesList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const mergeRemotePinsIntoFiles = (files, remotePins = [], pendingPins = [], fail
}

export const FilesList = ({
className, files, pins, pinningServices, remotePins, pendingPins, failedPins, filesSorting, updateSorting, downloadProgress, filesIsFetching, filesPathInfo, showLoadingAnimation,
className, files, pins, pinningServices, remotePins, pendingPins, failedPins, filesSorting, updateSorting, filesIsFetching, filesPathInfo, showLoadingAnimation,
onShare, onSetPinning, onInspect, onDownload, onRemove, onRename, onNavigate, onRemotePinClick, onAddFiles, onMove, doFetchRemotePins, doDismissFailedPin, handleContextMenuClick, t
}) => {
const [selected, setSelected] = useState([])
Expand Down Expand Up @@ -355,7 +355,6 @@ export const FilesList = ({
inspect={() => onInspect(selectedFiles[0].cid)}
count={selectedFiles.length}
isMfs={filesPathInfo.isMfs}
downloadProgress={downloadProgress}
size={selectedFiles.reduce((a, b) => a + (b.size || 0), 0)} />
}
</Fragment> }
Expand All @@ -374,7 +373,6 @@ FilesList.propTypes = {
asc: PropTypes.bool.isRequired
}),
updateSorting: PropTypes.func.isRequired,
downloadProgress: PropTypes.number,
filesIsFetching: PropTypes.bool,
filesPathInfo: PropTypes.object,
// Actions
Expand Down
30 changes: 2 additions & 28 deletions src/files/selected-actions/SelectedActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ class SelectedActions extends React.Component {
download: PropTypes.func.isRequired,
rename: PropTypes.func.isRequired,
inspect: PropTypes.func.isRequired,
downloadProgress: PropTypes.number,
t: PropTypes.func.isRequired,
tReady: PropTypes.bool.isRequired,
isMfs: PropTypes.bool.isRequired,
Expand All @@ -70,37 +69,12 @@ class SelectedActions extends React.Component {
force100: false
}

componentDidUpdate (prev) {
if (this.props.downloadProgress === 100 && prev.downloadProgress !== 100) {
this.setState({ force100: true })
setTimeout(() => {
this.setState({ force100: false })
}, 2000)
}
}

componentDidMount () {
this.containerRef.current && this.containerRef.current.focus()
}

get downloadText () {
if (this.state.force100) {
return this.props.t('finished')
}

if (!this.props.downloadProgress) {
return this.props.t('app:actions.download')
}

if (this.props.downloadProgress === 100) {
return this.props.t('finished')
}

return this.props.downloadProgress.toFixed(0) + '%'
}

render () {
const { t, tReady, animateOnStart, count, size, unselect, remove, share, setPinning, download, downloadProgress, rename, inspect, className, style, isMfs, ...props } = this.props
const { t, tReady, animateOnStart, count, size, unselect, remove, share, setPinning, download, rename, inspect, className, style, isMfs, ...props } = this.props
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we sort these alphabetically?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you know how to do it automatically, please don't make me do it 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint can do it automatically, we should not be forcing devs to do code style management


const isSingle = count === 1

Expand Down Expand Up @@ -131,7 +105,7 @@ class SelectedActions extends React.Component {
</button>
<button role="menuitem" className='tc mh2' onClick={download}>
<StrokeDownload className='w3 hover-fill-navy-muted' fill='#A4BFCC' aria-hidden="true"/>
<p className='ma0 f6'>{this.downloadText}</p>
<p className='ma0 f6'>{t('app:actions.download')}</p>
</button>
<button role="menuitem" className={classNames('tc mh2', classes.action(isMfs))} onClick={isMfs ? remove : null}>
<StrokeTrash className={classes.svg(isMfs)} fill='#A4BFCC' aria-hidden="true"/>
Expand Down
63 changes: 15 additions & 48 deletions src/lib/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,31 +35,20 @@ export function normalizeFiles (files) {
}

/**
* @typedef {Object} FileDownload
* @property {string} url
* @property {string} filename
* @property {string} method
*
* @param {FileStat} file
* @param {string} type
* @param {string} name
* @param {CID} cid
* @param {string} gatewayUrl
* @param {string} apiUrl
* @returns {Promise<FileDownload>}
* @returns {string}
*/
async function downloadSingle (file, gatewayUrl, apiUrl) {
let url, filename, method

if (file.type === 'directory') {
const name = file.name || `download_${file.cid}` // Name is not always available.
url = `${apiUrl}/api/v0/get?arg=${file.cid}&archive=true&compress=true`
filename = `${name}.tar.gz`
method = 'POST' // API is POST-only
function getDownloadURL (type, name, cid, gatewayUrl) {
if (type === 'directory') {
const filename = `${name || `download_${cid.toString()}`}.tar`
return `${gatewayUrl}/ipfs/${cid.toString()}?download=true&format=tar&filename=${filename}`
} else {
url = `${gatewayUrl}/ipfs/${file.cid}?download=true&filename=${file.name}`
filename = file.name
method = 'GET'
const filename = `${name || cid}`
return `${gatewayUrl}/ipfs/${cid.toString()}?download=true&filename=${filename}`
}

return { url, filename, method }
}

/**
Expand Down Expand Up @@ -87,42 +76,20 @@ export async function makeCIDFromFiles (files, ipfs) {
return stat.cid
}

/**
*
* @param {FileStat[]} files
* @param {string} apiUrl
* @param {IPFSService} ipfs
* @returns {Promise<FileDownload>}
*/
async function downloadMultiple (files, apiUrl, ipfs) {
if (!apiUrl) {
const e = new Error('api url undefined')
return Promise.reject(e)
}

const cid = await makeCIDFromFiles(files, ipfs)

return {
url: `${apiUrl}/api/v0/get?arg=${cid}&archive=true&compress=true`,
filename: `download_${cid}.tar.gz`,
method: 'POST' // API is POST-only
}
}

/**
*
* @param {FileStat[]} files
* @param {string} gatewayUrl
* @param {string} apiUrl
* @param {IPFSService} ipfs
* @returns {Promise<FileDownload>}
* @returns {Promise<string>}
*/
export async function getDownloadLink (files, gatewayUrl, apiUrl, ipfs) {
export async function getDownloadLink (files, gatewayUrl, ipfs) {
if (files.length === 1) {
return downloadSingle(files[0], gatewayUrl, apiUrl)
return getDownloadURL(files[0].type, files[0].name, files[0].cid, gatewayUrl)
}

return downloadMultiple(files, apiUrl, ipfs)
const cid = await makeCIDFromFiles(files, ipfs)
return getDownloadURL('directory', '', cid, gatewayUrl)
}

/**
Expand Down