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

fix uploadRemoteFile undefined #4814

Merged
merged 6 commits into from
Dec 12, 2023
Merged

fix uploadRemoteFile undefined #4814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a4df077
fix uploadRemoteFile undefined
mifi Dec 7, 2023
28bbc92
fix bugs
mifi Dec 8, 2023
f24bb96
remove unrelated space changes
aduh95 Dec 8, 2023
4e86b8d
apply suggestions
mifi Dec 8, 2023
c0bacc1
Update Uppy.js
mifi Dec 11, 2023
78c1d91
fix lint
mifi Dec 12, 2023
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
2 changes: 2 additions & 0 deletions examples/custom-provider/client/MyCustomProvider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export default class MyCustomProvider extends UIPlugin {
pluginId: this.id,
})

uppy.registerRequestClient(MyCustomProvider.name, this.provider)

this.defaultLocale = {
strings: {
pluginNameMyUnsplash: 'MyUnsplash',
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/aws-s3-multipart/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,7 +878,7 @@ export default class AwsS3Multipart extends BasePlugin {
}
this.uppy.on('file-removed', removedHandler)

const uploadPromise = file.remote.requestClient.uploadRemoteFile(
const uploadPromise = this.uppy.getRequestClientForFile(file).uploadRemoteFile(
file,
this.#getCompanionClientArgs(file),
{ signal: controller.signal, getQueue },
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/aws-s3/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ export default class AwsS3 extends BasePlugin {
}
this.uppy.on('file-removed', removedHandler)

const uploadPromise = file.remote.requestClient.uploadRemoteFile(
const uploadPromise = this.uppy.getRequestClientForFile(file).uploadRemoteFile(
file,
this.#getCompanionClientArgs(file),
{ signal: controller.signal, getQueue },
Expand Down
24 changes: 24 additions & 0 deletions packages/@uppy/core/src/Uppy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1386,6 +1386,30 @@ class Uppy {
}
}

// We need to store request clients by a unique ID, so we can share RequestClient instances across files
// this allows us to do rate limiting and synchronous operations like refreshing provider tokens
mifi marked this conversation as resolved.
Show resolved Hide resolved
// example: refreshing tokens: if each file has their own requestclient,
// we don't have any way to synchronize all requests in order to
// - block all requests
// - refresh the token
// - unblock all requests and allow them to run with a the new access token
// back when we had a requestclient per file, once an access token expired,
// all 6 files would go ahead and refresh the token at the same time
// (calling /refresh-token up to 6 times), which will probably fail for some providers
#requestClientById = new Map()

registerRequestClient(id, client) {
this.#requestClientById.set(id, client)
}

/** @protected */
getRequestClientForFile (file) {
if (!file.remote) throw new Error(`Tried to get RequestClient for a non-remote file ${file.id}`)
const requestClient = this.#requestClientById.get(file.remote.requestClientId)
if (requestClient == null) throw new Error(`requestClientId "${file.remote.requestClientId}" not registered for file "${file.id}"`)
return requestClient
}

/**
* Restore an upload by its ID.
*/
Expand Down
4 changes: 3 additions & 1 deletion packages/@uppy/provider-views/src/ProviderView/ProviderView.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ export default class ProviderView extends View {
isSearchVisible: false,
currentSelection: [],
})

this.registerRequestClient()
}

// eslint-disable-next-line class-methods-use-this
Expand Down Expand Up @@ -399,7 +401,7 @@ export default class ProviderView extends View {
// finished all async operations before we add any file
// see https://github.com/transloadit/uppy/pull/4384
this.plugin.uppy.log('Adding files from a remote provider')
this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file)))
this.plugin.uppy.addFiles(newFiles.map((file) => this.getTagFile(file, this.requestClientId)))

this.plugin.setPluginState({ filterInput: '' })
messages.forEach(message => this.plugin.uppy.info(message))
Expand Down
2 changes: 2 additions & 0 deletions packages/@uppy/provider-views/src/SearchProviderView/SearchProviderView.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ export default class SearchProviderView extends View {

// Set default state for the plugin
this.plugin.setPluginState(this.defaultState)

this.registerRequestClient()
}

// eslint-disable-next-line class-methods-use-this
Expand Down
12 changes: 6 additions & 6 deletions packages/@uppy/provider-views/src/View.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ export default class View {
uppy.info({ message, details: error.toString() }, 'error', 5000)
}

registerRequestClient() {
this.requestClientId = this.provider.provider;
this.plugin.uppy.registerRequestClient(this.requestClientId, this.provider)
}

// todo document what is a "tagFile" or get rid of this concept
getTagFile (file) {
const tagFile = {
Expand All @@ -78,15 +83,10 @@ export default class View {
},
providerName: this.provider.name,
provider: this.provider.provider,
requestClientId: this.requestClientId,
},
}

// all properties on this object get saved into the Uppy store.
// Some users might serialize their store (for example using JSON.stringify),
// or when using Golden Retriever it will serialize state into e.g. localStorage.
// However RequestClient is not serializable so we need to prevent it from being serialized.
Object.defineProperty(tagFile.remote, 'requestClient', { value: this.provider, enumerable: false })

const fileType = getFileType(tagFile)

// TODO Should we just always use the thumbnail URL if it exists?
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/tus/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ export default class Tus extends BasePlugin {
}
this.uppy.on('file-removed', removedHandler)

const uploadPromise = file.remote.requestClient.uploadRemoteFile(
const uploadPromise = this.uppy.getRequestClientForFile(file).uploadRemoteFile(
file,
this.#getCompanionClientArgs(file),
{ signal: controller.signal, getQueue },
Expand Down
8 changes: 6 additions & 2 deletions packages/@uppy/url/src/Url.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,16 @@ function getFileNameFromUrl (url) {
const { pathname } = new URL(url)
return pathname.substring(pathname.lastIndexOf('/') + 1)
}

/**
* Url
*
*/
export default class Url extends UIPlugin {
static VERSION = packageJson.version

static requestClientId = Url.name
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
static requestClientId = Url.name
static requestClientId = this.name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You sure that will work? doesn't this imply we have an instance? static happens before we have an instance right?


constructor (uppy, opts) {
super(uppy, opts)
this.id = this.opts.id || 'Url'
Expand Down Expand Up @@ -88,6 +91,8 @@ export default class Url extends UIPlugin {
companionHeaders: this.opts.companionHeaders,
companionCookiesRule: this.opts.companionCookiesRule,
})

this.uppy.registerRequestClient(Url.requestClientId, this.client)
Copy link
Member

Choose a reason for hiding this comment

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

Why only Url and not all other providers?

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 only url, see also changes in ProviderView and SearchProviderView

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment then why URL is the only exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, what i'm saying is that URL is not an exception. we do the same inside ProviderView and SearchProviderView

Copy link
Member

Choose a reason for hiding this comment

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

It is an exception because URL is apparently the only one that needs to have it explicitly, while the rest of the plugins have it implicitly through ProviderView?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not sure what the comment would say.

}

getMeta (url) {
Expand Down Expand Up @@ -132,11 +137,10 @@ export default class Url extends UIPlugin {
fileId: url,
url,
},
requestClientId: Url.requestClientId,
},
}

Object.defineProperty(tagFile.remote, 'requestClient', { value: this.client, enumerable: false })

this.uppy.log('[Url] Adding remote file')
try {
return this.uppy.addFile(tagFile)
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/xhr-upload/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ export default class XHRUpload extends BasePlugin {
}
this.uppy.on('file-removed', removedHandler)

const uploadPromise = file.remote.requestClient.uploadRemoteFile(
const uploadPromise = this.uppy.getRequestClientForFile(file).uploadRemoteFile(
file,
this.#getCompanionClientArgs(file),
{ signal: controller.signal, getQueue },
Expand Down