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

feat: update streaming dependencies to support react-native #895

Merged
merged 8 commits into from
Feb 10, 2020

Conversation

AllanZhengYP
Copy link
Contributor

@AllanZhengYP AllanZhengYP commented Feb 10, 2020

UPDATE: This PR update the dependency packages that uses API's not exist in ReactNative.

  1. chunked-blob-reader-rn: Equivalents to chunked-blob-reader package in browser except that it uses FileReader.readAsDataURL() instead of FileReader.readAsArrayBuffer(). The later one is not implemented in RN (code)
  2. stream-collector-rn: Equivalents to stream-collector-browser package in browser except not using FileReader.readAsArrayBuffer().
  3. fetch-http-handler: Add bufferBody config to the constructor. If it's set to true, the fetch-http-handler will buffer the whole response before returning. Hence, the returned Response.body will be a Blob shape instead of ReadableStream in browsers normally. So response streaming is not available in RN. When the option is set, fetch-http-handler uses Body.blob() under the hood to buffer the response.

This PR is tested with CognitoIdentity.GetId, S3.putObject, S3.deleteObject, S3.deleteObjects, S3.listObjects.

Resolves: #881 #874

This is a POC of adding RN support to SDK. Only tested with S3.listObjects. before setting up the local test environment, you need to manually publish all the relacted V3 SDK packages to a local npm server.

Test repo: https://github.com/AllanFly120/RNTestJSSDK

Screen Shot:
Simulator Screen Shot - iPhone X - 2020-02-09 at 16 21 17

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aws-sdk-js-automation

This comment has been minimized.

@codecov-io
Copy link

codecov-io commented Feb 10, 2020

Codecov Report

Merging #895 into smithy-codegen will increase coverage by 0.03%.
The diff coverage is 96%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           smithy-codegen     #895      +/-   ##
==================================================
+ Coverage           92.15%   92.18%   +0.03%     
==================================================
  Files                 148      149       +1     
  Lines                2905     2930      +25     
  Branches              513      516       +3     
==================================================
+ Hits                 2677     2701      +24     
- Misses                228      229       +1
Impacted Files Coverage Δ
packages/chunked-blob-reader-rn/src/index.ts 100% <100%> (ø)
...kages/fetch-http-handler/src/fetch-http-handler.ts 95.45% <50%> (-2.17%) ⬇️

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 ede0155...2fcd5ec. Read the comment docs.

@AllanZhengYP
Copy link
Contributor Author

AllanZhengYP commented Feb 10, 2020

Found an issue: the ETag on the wire is like
<ETag>&quot;841a2d689ad86bd1611447453c22c6fc&quot;</ETag>
In the v2, it is parsed into:
\"841a2d689ad86bd1611447453c22c6fc\"
In the v3 browser, it is parsed into:
"841a2d689ad86bd1611447453c22c6fc"
In the v3 ReactNative, it is parsed into:
&quot;841a2d689ad86bd1611447453c22c6fc&quot;

Need to dive deep on where do these discrepencies come from.

UPDATE: v3 browser is actually parsed same to v3 ReactNative:
&quot;841a2d689ad86bd1611447453c22c6fc&quot;
But it's still different to V2 behavior

@aws-sdk-js-automation

This comment has been minimized.

@AllanZhengYP AllanZhengYP changed the title [WIP]feat: initial commit to support react-native feat: initial commit to support react-native Feb 10, 2020
@AllanZhengYP AllanZhengYP marked this pull request as ready for review February 10, 2020 19:09
@AllanZhengYP AllanZhengYP changed the title feat: initial commit to support react-native feat: update streaming dependencies to support react-native Feb 10, 2020
@aws-sdk-js-automation

This comment has been minimized.

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@AllanZhengYP AllanZhengYP merged commit 634b3f6 into aws:smithy-codegen Feb 10, 2020
@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@lock
Copy link

lock bot commented Feb 17, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 17, 2020
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.

None yet

4 participants