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

@uppy/core: Don't set late (throttled) progress event on a file that is 100% complete #4507

Merged
merged 1 commit into from
Jun 19, 2023
Merged

@uppy/core: Don't set late (throttled) progress event on a file that is 100% complete #4507

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 17 additions & 13 deletions packages/@uppy/core/src/Uppy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,6 @@ class Uppy {

this.i18nInit()

// ___Why throttle at 500ms?
// - We must throttle at >250ms for superfocus in Dashboard to work well
// (because animation takes 0.25s, and we want to wait for all animations to be over before refocusing).
// [Practical Check]: if thottle is at 100ms, then if you are uploading a file,
// and click 'ADD MORE FILES', - focus won't activate in Firefox.
// - We must throttle at around >500ms to avoid performance lags.
// [Practical Check] Firefox, try to upload a big file for a prolonged period of time. Laptop will start to heat up.
this.calculateProgress = throttle(this.calculateProgress.bind(this), 500, { leading: true, trailing: true })

this.store = this.opts.store
this.setState({
plugins: {},
Expand Down Expand Up @@ -893,17 +884,30 @@ class Uppy {
})
}

calculateProgress (file, data) {
if (file == null || !this.getFile(file.id)) {
// ___Why throttle at 500ms?
// - We must throttle at >250ms for superfocus in Dashboard to work well
// (because animation takes 0.25s, and we want to wait for all animations to be over before refocusing).
// [Practical Check]: if thottle is at 100ms, then if you are uploading a file,
// and click 'ADD MORE FILES', - focus won't activate in Firefox.
// - We must throttle at around >500ms to avoid performance lags.
// [Practical Check] Firefox, try to upload a big file for a prolonged period of time. Laptop will start to heat up.
calculateProgress = throttle((file, data) => {
const fileInState = this.getFile(file?.id)
if (file == null || !fileInState) {
this.log(`Not setting progress for a file that has been removed: ${file?.id}`)
return
}

if (fileInState.progress.percentage === 100) {
this.log(`Not setting progress for a file that has been already uploaded: ${file.id}`)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to log this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do above, so why not here? This way we know something is up, ideally should not be happening. When you set debug: false in prod, you don't see any of those logs, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

should we >= 100 just to be sure?

return
}

// bytesTotal may be null or zero; in that case we can't divide by it
const canHavePercentage = Number.isFinite(data.bytesTotal) && data.bytesTotal > 0
this.setFileState(file.id, {
progress: {
...this.getFile(file.id).progress,
...fileInState.progress,
bytesUploaded: data.bytesUploaded,
bytesTotal: data.bytesTotal,
percentage: canHavePercentage
Expand All @@ -913,7 +917,7 @@ class Uppy {
})

this.calculateTotalProgress()
}
}, 500, { leading: true, trailing: true })

calculateTotalProgress () {
// calculate total progress, using the number of files currently uploading,
Expand Down