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.

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

@uppy/xhr-upload: add 'upload-stalled' event #4247

Merged
merged 10 commits into from
Feb 1, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 13, 2022

Instead of cancelling the upload, let's give the user control over what to do when a stalling is detected.

The stall detection was added in #378, but using a hard-coded time limit is probably not a good idea.

@aduh95 aduh95 changed the title @uppy/xhr-upload: add'upload-stalled' event @uppy/xhr-upload: add 'upload-stalled' event Dec 13, 2022
@aduh95 aduh95 linked an issue Dec 13, 2022 that may be closed by this pull request
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

I like the idea!

But the event needs an complementary example because I'm not sure how to act on it.

website/src/docs/xhr-upload.md Outdated Show resolved Hide resolved
website/src/docs/core.md Outdated Show resolved Hide resolved
Co-authored-by: Merlijn Vos <merlijn@soverin.net>
@Murderlon
Copy link
Member

Note that any changes made to the docs must also happen in https://github.com/transloadit/uppy.io

website/src/docs/core.md Outdated Show resolved Hide resolved
Co-authored-by: Merlijn Vos <merlijn@soverin.net>
website/src/docs/core.md Outdated Show resolved Hide resolved
website/src/docs/core.md Outdated Show resolved Hide resolved
uppy.off('upload-progress', uploadProgressHandler)
}
}
uppy.on('upload-progress', uploadProgressHandler)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to set this handler 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.

It's to illustrate how to detect when the upload is no longer stalled.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we can add a comment about it. Or actually show an example that informs the user, as that's what we talk about above

"Use this event to display a message on the UI to tell the user they might want to retry the upload."

Now it's a bit confusing to me how retrying works together with 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.

I've updated the variable name so it's clearer, PTAL

let uploadStalledWarningRecentlyEmitted
this.on('upload-stalled', (error, files) => {
const { message } = error
const details = files.map(file => file.meta.name).join(', ')
Copy link
Member

Choose a reason for hiding this comment

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

Think is going to look bad as soon as you have more than 10 files. I would say this is a general problem in informer. It's probably best if we could inline errors per file always and use infomer only for generic updates. Any chance we can reuse the meta data validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the details is not shown by default, it's only visible if the user click on the ? on the informer bubble.

@arturi
Copy link
Contributor

arturi commented Dec 24, 2022

Tested with local XHR server and cutting off internet in the middle of the upload, both bundle and non-bundle work. Bundle lists all the files in the (?), non-bundle only shows one error with one file in the (?).

Screenshot from 2022-12-24 01-53-30:

Screenshot from 2022-12-24 01-55-42

@arturi
Copy link
Contributor

arturi commented Dec 24, 2022

Still wasn't able to upload anything slow to Heroku:

Screenshot from 2022-12-24 01-34-56

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Jan 31, 2023
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels Jan 31, 2023
@arturi arturi merged commit be85449 into transloadit:main Feb 1, 2023
@github-actions github-actions bot mentioned this pull request Feb 13, 2023
github-actions bot added a commit that referenced this pull request Feb 13, 2023
| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/audio          |   1.0.4 | @uppy/screen-capture |   3.0.2 |
| @uppy/companion      |   4.3.0 | @uppy/transloadit    |   3.1.1 |
| @uppy/core           |   3.0.6 | @uppy/xhr-upload     |   3.1.0 |
| @uppy/dashboard      |   3.2.2 | uppy                 |   3.5.0 |
| @uppy/locales        |   3.0.6 |                      |         |

- @uppy/transloadit: fix `assemblyOptions` option (Antoine du Hamel / #4316)
- meta: Remove Robodog advice, since it is deprecated (Artur Paikin)
- @uppy/dashboard: fix dashboard acquirers list (Mikael Finstad / #4306)
- @uppy/dashboard: Dashboard: disallow clicking on buttons and links in Dashboard disabled mode (Artur Paikin / #4292)
- @uppy/audio,@uppy/core,@uppy/dashboard,@uppy/screen-capture: Warn more instead of erroring (Artur Paikin / #4302)
- @uppy/locales: Update de_DE.js (Jörn Velten / #4297)
- meta: use load balancer for companion in e2e tests (Mikael Finstad / #4228)
- @uppy/companion: @uppy/companion upgrade grant dependency (Scott Bessler / #4286)
- @uppy/xhr-upload: add `'upload-stalled'` event (Antoine du Hamel / #4247)
- @uppy/locales: minor enhancements and typo fixes for the hungarian translation (KergeKacsa / #4282)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package              | Version | Package              | Version |
| -------------------- | ------- | -------------------- | ------- |
| @uppy/audio          |   1.0.4 | @uppy/screen-capture |   3.0.2 |
| @uppy/companion      |   4.3.0 | @uppy/transloadit    |   3.1.1 |
| @uppy/core           |   3.0.6 | @uppy/xhr-upload     |   3.1.0 |
| @uppy/dashboard      |   3.2.2 | uppy                 |   3.5.0 |
| @uppy/locales        |   3.0.6 |                      |         |

- @uppy/transloadit: fix `assemblyOptions` option (Antoine du Hamel / transloadit#4316)
- meta: Remove Robodog advice, since it is deprecated (Artur Paikin)
- @uppy/dashboard: fix dashboard acquirers list (Mikael Finstad / transloadit#4306)
- @uppy/dashboard: Dashboard: disallow clicking on buttons and links in Dashboard disabled mode (Artur Paikin / transloadit#4292)
- @uppy/audio,@uppy/core,@uppy/dashboard,@uppy/screen-capture: Warn more instead of erroring (Artur Paikin / transloadit#4302)
- @uppy/locales: Update de_DE.js (Jörn Velten / transloadit#4297)
- meta: use load balancer for companion in e2e tests (Mikael Finstad / transloadit#4228)
- @uppy/companion: @uppy/companion upgrade grant dependency (Scott Bessler / transloadit#4286)
- @uppy/xhr-upload: add `'upload-stalled'` event (Antoine du Hamel / transloadit#4247)
- @uppy/locales: minor enhancements and typo fixes for the hungarian translation (KergeKacsa / transloadit#4282)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XHRUpload upload stalled for 30s
3 participants