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.

Sign up for GitHub

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

Jump to bottom

Companion: sort Dropbox response & refactor to async/await #3897

Merged
merged 4 commits into from Aug 4, 2022
Merged

Companion: sort Dropbox response & refactor to async/await #3897

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
77035eb
rewrite dropbox to async/await
mifi Jul 24, 2022
c355cad
implement sort
mifi Jul 24, 2022
3b26d7e
revert modern features
mifi Jul 24, 2022
f893414
Merge branch 'main' into dropbox-sort
aduh95 Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions packages/@uppy/companion/src/server/provider/dropbox/adapter.js
View file
Open in desktop
@@ -1,10 +1,6 @@
const mime = require('mime-types')
const querystring = require('querystring')

exports.getUsername = (data) => {
return data.user_email
}

exports.isFolder = (item) => {
return item['.tag'] === 'folder'
}
Expand Down
161 changes: 65 additions & 96 deletions packages/@uppy/companion/src/server/provider/dropbox/index.js
View file
Open in desktop
Expand Up @@ -20,6 +20,27 @@ function httpHeaderSafeJson (v) {
})
}

function adaptData (res, email, buildURL) {
const items = adapter.getItemSubList(res).map((item) => ({
isFolder: adapter.isFolder(item),
icon: adapter.getItemIcon(item),
name: adapter.getItemName(item),
mimeType: adapter.getMimeType(item),
id: adapter.getItemId(item),
thumbnail: buildURL(adapter.getItemThumbnailUrl(item), true),
requestPath: adapter.getItemRequestPath(item),
modifiedDate: adapter.getItemModifiedDate(item),
size: adapter.getItemSize(item),
}))
items.sort((a, b) => a.name.localeCompare(b.name, 'en-US', { numeric: true }))
Copy link
Member

Choose a reason for hiding this comment

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

This works too, but I'm sure the Dropbox API supports a query parameter to do it for us. Either way is fine I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like it doesnt: https://www.dropboxforum.com/t5/Dropbox-API-Support-Feedback/list-orderby-api/td-p/337393

Comment on lines +34 to +35
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}))
items.sort((a, b) => a.name.localeCompare(b.name, 'en-US', { numeric: true }))
})).sort((a, b) => a.name.localeCompare(b.name, 'en-US', { numeric: true }))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it reads better on a separate line

Copy link
Member

Choose a reason for hiding this comment

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

I don't, and also it's not immediately obvious that .sort would modify the original array if you're not familiar. I won't push it if that's your preference though.


return {
username: email,
items,
nextPagePath: adapter.getNextPagePath(res),
}
}

/**
* Adapter for API https://www.dropbox.com/developers/documentation/http/documentation
*/
Expand All @@ -39,77 +60,48 @@ class DropBox extends Provider {
return 'dropbox'
}

_userInfo ({ token }, done) {
this.client
async _userInfo ({ token }) {
const client = this.client
.post('users/get_current_account')
.options({ version: '2' })
.auth(token)
.request(done)
return promisify(client.request.bind(client))()
}

/**
* Makes 2 requests in parallel - 1. to get files, 2. to get user email
* it then waits till both requests are done before proceeding with the callback
*
* @param {object} options
* @param {Function} done
*/
_list (options, done) {
let userInfoDone = false
let statsDone = false
let userInfo
let stats
let reqErr
const finishReq = () => {
if (reqErr) {
logger.error(reqErr, 'provider.dropbox.list.error')
done(reqErr)
} else {
stats.body.user_email = userInfo.body.email
done(null, this.adaptData(stats.body, options.companion))
}
async list (options) {
try {
const responses = await Promise.all([
this._stats(options),
this._userInfo(options),
])
responses.forEach((response) => {
if (response.statusCode !== 200) throw this._error(null, response)
})
const [{ body: stats }, { body: { email } }] = responses
Copy link
Member

Choose a reason for hiding this comment

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

nit: triple destructering hurts readability

Copy link
Contributor Author

@mifi mifi Jul 25, 2022 •
edited

Choose a reason for hiding this comment

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

Suggested change
const [{ body: stats }, { body: { email } }] = responses
const { body: stats } = responses[0]
const { body: { email } } = responses[1]

return adaptData(stats, email, options.companion.buildURL)
} catch (err) {
logger.error(err, 'provider.dropbox.list.error')
throw err
}

this.stats(options, (err, resp) => {
statsDone = true
stats = resp
if (err || resp.statusCode !== 200) {
err = this._error(err, resp)
}
reqErr = reqErr || err
if (userInfoDone) {
finishReq()
}
})

this._userInfo(options, (err, resp) => {
userInfoDone = true
userInfo = resp
if (err || resp.statusCode !== 200) {
err = this._error(err, resp)
}

reqErr = reqErr || err
if (statsDone) {
finishReq()
}
})
}

stats ({ directory, query, token }, done) {
async _stats ({ directory, query, token }) {
if (query.cursor) {
this.client
const client = this.client
.post('files/list_folder/continue')
.options({ version: '2' })
.auth(token)
.json({
cursor: query.cursor,
})
.request(done)
return
return promisify(client.request.bind(client))()
Copy link
Member

Choose a reason for hiding this comment

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

Intead, let's pass promise:true when creating the purest instance here

uppy/packages/@uppy/companion/src/server/provider/dropbox/index.js

Line 2 in 7b0db7d

const purest = require('purest')({ request })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure i follow

Copy link
Member

@aduh95 aduh95 Jul 27, 2022 •
edited

Choose a reason for hiding this comment

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

purest supports promises out of the box, let's use that instead of promisifying their API over and over again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i cannot find any promise:true option documented anywhere. are you sure it's for purest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, now i see, v4 uses promises by default. maybe we can try to upgrade to purest v4. or v3 uses require('purest')({ request, promise: Promise })

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok, I've seen the promise option and assumed it was a boolean. Yes, let's upgrade, they've dropped support for Node.js 10.x but have we on 3.x.

Copy link
Member

Choose a reason for hiding this comment

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

If we're not upgrading or migrating just yet, can we use the promise option?

}

this.client
const client = this.client
.post('files/list_folder')
.options({ version: '2' })
.qs(query)
Expand All @@ -118,7 +110,8 @@ class DropBox extends Provider {
path: `${directory || ''}`,
include_non_downloadable_files: false,
})
.request(done)

return promisify(client.request.bind(client))()
}

async download ({ id, token }) {
Expand Down Expand Up @@ -160,57 +153,37 @@ class DropBox extends Provider {
}
}

_size ({ id, token }, done) {
return this.client
async size ({ id, token }) {
const client = this.client
.post('files/get_metadata')
.options({ version: '2' })
.auth(token)
.json({ path: id })
.request((err, resp, body) => {
if (err || resp.statusCode !== 200) {
err = this._error(err, resp)
logger.error(err, 'provider.dropbox.size.error')
return done(err)
}
done(null, parseInt(body.size, 10))
})

try {
const resp = await promisify(client.request.bind(client))()
if (resp.statusCode !== 200) throw this._error(null, resp)
return parseInt(resp.body.size, 10)
} catch (err) {
logger.error(err, 'provider.dropbox.size.error')
throw err
}
}

_logout ({ token }, done) {
return this.client
async logout ({ token }) {
const client = this.client
.post('auth/token/revoke')
.options({ version: '2' })
.auth(token)
.request((err, resp) => {
if (err || resp.statusCode !== 200) {
logger.error(err, 'provider.dropbox.logout.error')
done(this._error(err, resp))
return
}
done(null, { revoked: true })
})
}

adaptData (res, companion) {
const data = { username: adapter.getUsername(res), items: [] }
const items = adapter.getItemSubList(res)
items.forEach((item) => {
data.items.push({
isFolder: adapter.isFolder(item),
icon: adapter.getItemIcon(item),
name: adapter.getItemName(item),
mimeType: adapter.getMimeType(item),
id: adapter.getItemId(item),
thumbnail: companion.buildURL(adapter.getItemThumbnailUrl(item), true),
requestPath: adapter.getItemRequestPath(item),
modifiedDate: adapter.getItemModifiedDate(item),
size: adapter.getItemSize(item),
})
})

data.nextPagePath = adapter.getNextPagePath(res)

return data
try {
const resp = await promisify(client.request.bind(client))()
if (resp.statusCode !== 200) throw this._error(null, resp)
return { revoked: true }
} catch (err) {
logger.error(err, 'provider.dropbox.logout.error')
throw err
}
}

_error (err, resp) {
Expand All @@ -226,8 +199,4 @@ class DropBox extends Provider {

DropBox.version = 2

DropBox.prototype.list = promisify(DropBox.prototype._list)
DropBox.prototype.size = promisify(DropBox.prototype._size)
DropBox.prototype.logout = promisify(DropBox.prototype._logout)

module.exports = DropBox