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

Adding download progress tracker for Storage.get #8295

Merged
merged 18 commits into from
Jul 7, 2021

Conversation

jamesaucode
Copy link
Contributor

Description of changes

This adds a download tracker for .get similar to .put. Also changed SEND_PROGRESS to SEND_UPLOAD_PROGRESS to remove ambiguity.
Added a small test to verify the cancelToken from the last PR's .cancel API gets properly added to the request

Doc change PR in #3238

Issue #, if available

fixes #4734

Description of how you validated changes

Added relevant unit tests, manually tested.

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2021

Codecov Report

Merging #8295 (ff26889) into main (a84978d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8295   +/-   ##
=======================================
  Coverage   77.96%   77.96%           
=======================================
  Files         237      237           
  Lines       16824    16836   +12     
  Branches     3614     3616    +2     
=======================================
+ Hits        13116    13126   +10     
- Misses       3582     3584    +2     
  Partials      126      126           
Impacted Files Coverage Δ
packages/storage/src/providers/AWSS3Provider.ts 96.76% <100.00%> (+0.09%) ⬆️
...torage/src/providers/AWSS3ProviderManagedUpload.ts 90.19% <100.00%> (+0.06%) ⬆️
...ckages/storage/src/providers/axios-http-handler.ts 100.00% <100.00%> (ø)
...kages/amazon-cognito-identity-js/src/BigInteger.js 89.31% <0.00%> (-0.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a84978d...ff26889. Read the comment docs.

@jamesaucode jamesaucode changed the title Storage/download progress Adding download progress tracker for Storage.get May 18, 2021
@jamesaucode jamesaucode requested a review from amhinson May 19, 2021 17:28
@jamesaucode jamesaucode requested a review from a team May 19, 2021 18:30
Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

Pulled and tested (manually). Seems to work as advertised!

Is there an associated integ test PR?

@jamesaucode
Copy link
Contributor Author

Pulled and tested (manually). Seems to work as advertised!

Is there an associated integ test PR?

Thanks! Not yet, will add it and reference it here!

@elorzafe elorzafe requested review from elorzafe and removed request for amhinson July 2, 2021 16:49
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

This is looking good @jamesaucode !

I have a couple of questions if you can review those

Thanks again!

@@ -265,6 +271,18 @@ export class AWSS3Provider implements StorageProvider {
if (download === true) {
const getObjectCommand = new GetObjectCommand(params);
try {
if (progressCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is axios downloader not the browser downloader right?, where is this stored I don't remember how this worked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is for the use case of when user calls Storage.get('key', { download:true }) where the user wants the Blob directly.

progressCallback will be called whenever the onDownloadProgress event fires from axios (which seems to be the progress event from XMLHttpRequest https://github.com/axios/axios/blob/master/lib/adapters/xhr.js#L159)

@jamesaucode jamesaucode merged commit 8fe1853 into aws-amplify:main Jul 7, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2022

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage.get - Download progress tracking
6 participants