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

Upgrade aws-sdk to gamma version #5854

Merged
merged 9 commits into from
May 22, 2020

Conversation

Amplifiyer
Copy link
Contributor

@Amplifiyer Amplifiyer commented May 21, 2020

Issue #, if available:

Description of changes:

Note: The build won't succeed until aws-sdk release their gamma version. I've been testing with a local copy of cached verdaccio release.

This PR is to upgrade aws-sdk version which fixes the above mentioned amplify issues. There are some compatibility changes since this release of aws-sdk is not 100% backwards compatible.

  1. Browser environment now requires the http response to be either a blob or a stream. Updated axios http handler to now always request a blob and return it. This also makes it consistent for react-native and browsers environment to always return blobs to amplify customers. Need to discuss since this would be a breaking change. See table below for reference
Release Browser React Native
Amplify V2 UInt8Array UInt8Array
Amplify V3 String (#5393) Blob
This PR Blob Blob
  1. Local mock storage now requires another parameter tls to enforce http endpoints.

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

@Amplifiyer Amplifiyer requested a review from iartemiev as a code owner May 21, 2020 03:17
@lgtm-com
Copy link

lgtm-com bot commented May 21, 2020

This pull request introduces 1 alert when merging 48bf251 into e71fee5 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@lgtm-com
Copy link

lgtm-com bot commented May 21, 2020

This pull request introduces 1 alert when merging 986b74b into f89e545 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@sammartinez sammartinez added needs-review Core Related to core Amplify issues labels May 21, 2020
@lgtm-com
Copy link

lgtm-com bot commented May 21, 2020

This pull request introduces 1 alert when merging 8a27e12 into 24eb104 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #5854 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5854      +/-   ##
==========================================
- Coverage   73.75%   73.75%   -0.01%     
==========================================
  Files         204      203       -1     
  Lines       11973    11970       -3     
  Branches     2254     2254              
==========================================
- Hits         8831     8828       -3     
  Misses       2980     2980              
  Partials      162      162              
Impacted Files Coverage Δ
packages/storage/src/providers/AWSS3Provider.ts 87.30% <100.00%> (ø)
...torage/src/providers/AWSS3ProviderManagedUpload.ts 95.10% <100.00%> (-0.04%) ⬇️
...ckages/storage/src/providers/axios-http-handler.ts 75.00% <100.00%> (-0.52%) ⬇️

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 3b2ec1b...0733a83. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented May 21, 2020

This pull request introduces 1 alert when merging 73dfe26 into 3b2ec1b - view on LGTM.com

new alerts:

  • 1 for Useless conditional

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.

Nice @Amplifiyer !!

My only comment as we talked before, is to change the return type of Storage.get when download: true that might need to be overloaded probably to set the correct type when download: false (url)

s3BucketEndpoint: true,
s3ForcePathStyle: true,
tls: false,
bucketEndpoint: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was bucketEndpoint changed to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense!

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 call this out in the readme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a 100% non-breaking change for Amplify customers

@Amplifiyer
Copy link
Contributor Author

is to change the return type of Storage.get when download: true

Thanks @elorzafe. The type that's changing is the result.Body. Is there any trick shortcut to specify a type definition that explicitly has one member of a specific type but otherwise generic?

Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jordanranz jordanranz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

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

lgtm

@Amplifiyer Amplifiyer merged commit ca02b50 into aws-amplify:master May 22, 2020
@masbaehr
Copy link

Just for reference: This update to gamma cause this critical issue, as all S3 uploads stopped working in Chrome:

#5873

@github-actions
Copy link

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 Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core Related to core Amplify issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants