refactor: Support all the webs #1123

russell-dot-js · 2020-04-29T03:03:34Z

Collapse stream-collector-browser and stream-collector-native into stream-collector-web
Update FetchHttpHandler to return blob if stream not implemented

Issue #, if available:
#1107

Description of changes:

Remove stream-collector-browser and stream-collector-native in favor of generic stream-collector-web
Update FetchHttpHandler to always return blob if stream not implemented

The real changes are here and (starting) here. Rest is find-replace.

Open questions

Naming of stream-collector-web? I would normally call it stream-collector-client, considering it's for use client-side, but don't want it to be easily confused with the actual (AWS) clients in the sdk. Could also go for something like stream-collector-fetch to demonstrate coupling with FetchHttpHandler
Ok that since, this is technically a new package, I started it at 1.0.0-beta.0?
What if we collapsed the "HttpHandlers" and "StreamCollectors" into a single package? E.g. rather than mixing and matching HttpHandlers and StreamCollectors, they always come together. So if you use FetchHttpHandler, you get the FetchStreamCollector. etc.
Can we get rid of the "bufferBody" option in FetchHttpHandler? There's no reason for native to say "never give me a stream" - my assumption is that it will be supported, eventually, and as long as we support both scenarios at run-time, we're good to go.
Is there a preference to keep stream-collector-browser and stream-collector-native separate, even though they'd both delegate to this same package? My thought was that since this is still in beta, and we can get away with deleting stuff, I didn't see a reason to create (what's arguably) technical debt.

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 · 2020-04-29T03:05:42Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: cb7e413
Result: FAILED
Build Logs (available for 30 days)

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

aws-sdk-js-automation · 2020-04-29T03:07:39Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: 41634df
Result: FAILED
Build Logs (available for 30 days)

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

russell-dot-js · 2020-04-29T03:16:38Z

Crap. I forgot to update stream-collector-web/CHANGELOG after the git mv from stream-collector-native. Will wait for feedback to open questions in PR description before buttoning up.

aws-sdk-js-automation · 2020-04-29T03:20:13Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: d455ef9
Result: FAILED
Build Logs (available for 30 days)

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

aws-sdk-js-automation · 2020-04-29T03:34:24Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: 0e6f077
Result: FAILED
Build Logs (available for 30 days)

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

aws-sdk-js-automation · 2020-04-29T03:37:22Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: d638485
Result: FAILED
Build Logs (available for 30 days)

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

AllanZhengYP

The new name stream-collector-web sounds good to me. Will need @trivikr for another look. We also need to confirm whether removing bufferBody config from fetch-http-handler will affect Amplify(I don't think so).

AllanZhengYP · 2020-04-29T18:35:44Z

packages/fetch-http-handler/src/fetch-http-handler.ts

+  // TODO can we get rid of this? is there a scenario where a client wants to
+  //  insist on a Blob, even if stream is implemented? regardless, comment above
+  //  is outdated, this is not only useful in ReactNative due to variety of fetch
+  //  implementations


I think the same way. If we rely on dynamicly detecting the ReadableStream implementation in the environment, we should remove it.

AllanZhengYP · 2020-04-29T18:51:43Z

Hey @russell-dot-js

Very awesome PR! Regarding your questions:

Is there a preference to keep stream-collector-browser and stream-collector-native separate, even though they'd both delegate to this same package? My thought was that since this is still in beta, and we can get away with deleting stuff, I didn't see a reason to create (what's arguably) technical debt.

Given we prefer to return body in blob or ReadableStream dynamicly from fetch-http-handler, we should also merge the 2 difference stream collector.

What if we collapsed the "HttpHandlers" and "StreamCollectors" into a single package? E.g. rather than mixing and matching HttpHandlers and StreamCollectors, they always come together. So if you use FetchHttpHandler, you get the FetchStreamCollector. etc.

Unfortunately we cannot do it. For any service/operations supporting streaming payload, we don't call StreamCollectors to the response, instead we return the stream from low-level http handler to users directly. In these case, the http handlers doesn't have the prior knowledge of whether the response is from an operation with streaming member.

Generally very solid PR. I'd like to get some more set of eyes the review the solution. Ones we decide to go with this solution, I can provide some help with the code generation. Basically, we don't work directly on clients folder, because they are generated. Instead, we make code change to our code generator in Java, or the lower level smithy typescript generator. Don't worry about it just now, we will provide all the guides needed.

russell-dot-js · 2020-04-29T20:46:03Z

Hey @russell-dot-js

Very awesome PR!

Thank you!! Happy to help.

Is there a preference to keep stream-collector-browser and stream-collector-native separate, even though they'd both delegate to this same package? My thought was that since this is still in beta, and we can get away with deleting stuff, I didn't see a reason to create (what's arguably) technical debt.

Given we prefer to return body in blob or ReadableStream dynamicly from fetch-http-handler, we should also merge the 2 difference stream collector.

Noted

What if we collapsed the "HttpHandlers" and "StreamCollectors" into a single package? E.g. rather than mixing and matching HttpHandlers and StreamCollectors, they always come together. So if you use FetchHttpHandler, you get the FetchStreamCollector. etc.

Unfortunately we cannot do it. For any service/operations supporting streaming payload, we don't call StreamCollectors to the response, instead we return the stream from low-level http handler to users directly. In these case, the http handlers doesn't have the prior knowledge of whether the response is from an operation with streaming member.

Curious -- are you sure we can't do this?

Example:

interface HttpProvider<T> {
  handler: HttpHandler<T>;
  streamCollector: StreamCollector<T>;
}

// could be generic or not, doesn't really matter
// T might be ReadableStream | Blob for web, or Readable for node.

const webHttpProvider: HttpProvider<ReadableStream | Blob> = {
  handler: fetchHttpHandler,
  streamCollector: streamCollectorWeb,
};

const nodeHttpProvider: HttpProvider<Readable> = {
  handler: nodeHttpHandler,
  streamCollector: streamCollectorNode,
};

// clients would have single arg httpProvider, rather than args 
// httpHandler and streamCollector.

For the S3 client, the streamCollector would be ignored, but for most clients, it would be used.

Generally very solid PR. I'd like to get some more set of eyes the review the solution. Ones we decide to go with this solution, I can provide some help with the code generation. Basically, we don't work directly on clients folder, because they are generated. Instead, we make code change to our code generator in Java, or the lower level smithy typescript generator. Don't worry about it just now, we will provide all the guides needed.

Got it! I knew there was some code gen for the clients, but assumed that since the clients were checked in, they were not code-genned. For my own understanding, can you please clarify why the generated code is checked in? Is this to speed up CI, to keep every change in version control to ease human debugging, or to facilitate one-off edits of generated code (seems risky)?

Also, I am curious why I have tests failing in CI that did not pass locally. yarn test:all passed locally... perhaps I should open an issue regarding unclear documentation around how to test? I'm also happy to look into #1092, adding more transparency to the CI process would help "outsiders" like me contribute.

russell-dot-js · 2020-04-29T21:18:57Z

@AllanFly120 I've amended my commit (do you prefer squashed PR's?) removing "bufferBody", and fixing the failing tests. I had to update the tests to not return "body", as the tests were returning strings, not ReadableStreams, and we do not have a ReadableStream polyfill set up for the node test environment (do we want one? It would be nice to add some unit tests demonstrating that it properly handles ReadableStream if implemented, and falls back to blob.)

See changes in packages/fetch-http-handler/src/fetch-http-handler.ts and packages/fetch-http-handler/src/fetch-http-handler.spec.ts

Also, regarding my question about yarn test:all -- it looks like test:all does not actually run all tests, it is defined as
"test:all": "jest --coverage --passWithNoTests && lerna run test --scope @aws-sdk/stream-collector-web --scope @aws-sdk/hash-blob-browser",

Were those scopes accidentally committed, or is the intent of test:all to only test those two packages?

aws-sdk-js-automation · 2020-04-29T21:33:21Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: ba7274e
Result: SUCCEEDED
Build Logs (available for 30 days)

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

russell-dot-js · 2020-05-06T19:05:40Z

@AllanFly120 / @trivikr just wanted to check in on this? Per #1121 (comment), @Stereobit also has users in a tough spot right now. Maybe we can at least release the quick fix in #1121 while this is reviewed?

Correspond to aws/aws-sdk-js-v3#1123

AllanZhengYP · 2020-05-12T15:58:44Z

@russell-dot-js Hi, sorry for the late response. Can you remove all the diffs in clients folder from this PR and only keep the fetch-http-handler and event-collector-web changes? I will open another PR with updated clients to coordinate with this PR.

AllanZhengYP · 2020-05-12T07:20:08Z

packages/fetch-http-handler/src/fetch-http-handler.ts

+        //   can we just check for existence of response.body, as that should be
+        //   guaranteed to be a ReadableStream if it exists?
+        const hasResponseStream = typeof ReadableStream === 'function' &&
+          response.body instanceof ReadableStream;


This is a good point. I agree that we can just check response.body for now.

Done and done

Was hoping you could response to my questions here:
#1123 (comment)
and here?
#1123 (comment)

Correspond to aws/aws-sdk-js-v3#1123

* Collapse stream-collector-browser and stream-collector-native into stream-collector-web * Update FetchHttpHandler to return blob if stream not implemented

codecov-io · 2020-05-12T21:01:15Z

Codecov Report

Merging #1123 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1123      +/-   ##
==========================================
- Coverage   73.54%   73.53%   -0.01%     
==========================================
  Files         283      284       +1     
  Lines       13014    13023       +9     
  Branches     3030     3035       +5     
==========================================
+ Hits         9571     9577       +6     
- Misses       3443     3446       +3

Impacted Files	Coverage Δ
...kages/fetch-http-handler/src/fetch-http-handler.ts	`93.47% <0.00%> (-1.98%)`	⬇️
packages/util-body-length-browser/src/index.ts	`71.42% <0.00%> (ø)`

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 65a3658...0bc10e8. Read the comment docs.

aws-sdk-js-automation · 2020-05-12T21:03:42Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: 0eedacd
Result: FAILED
Build Logs (available for 30 days)

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

aws-sdk-js-automation · 2020-05-12T21:05:42Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: 0bc10e8
Result: FAILED
Build Logs (available for 30 days)

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

aws-sdk-js-automation · 2020-05-12T21:24:36Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: 86dd467
Result: FAILED
Build Logs (available for 30 days)

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

…l-the-browsers

aws-sdk-js-automation · 2020-05-12T21:36:01Z

AWS CodeBuild CI Report

CodeBuild project: sdk-staging-test
Commit ID: fb381d9
Result: FAILED
Build Logs (available for 30 days)

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

AllanZhengYP · 2020-05-12T23:02:04Z

Hi @russell-dot-js

Curious -- are you sure we can't do this?

After putting more thoughts on it I think:

✅ we can colapse fetch-request-handler and stream-collector-web into the same package; Also node-http-handler and stream-collector-node into the same package. Because they always co-exist.
❌ we should not put the handler and stream collector under the same interface(e.g. nodeHttpProvider). Because they belong to different request handling stage and should be decoupled for extensibility of SDK, here's details:

Request handler are supposed to be a thin layer over the runtime primitives, it must return the raw low level response payload. It serves as the core of service client's middleware stack regardless of the service model. However, stream collector is a util function, although very commonly used, that defined by the service model. It is generated when the service needs structured response.

Why is this difference important? Because V3 SDK is designed with a lot of flexibility. Any AWS client is extended from a white-labeled client that only knows the low-level request handler(requestHandler). This design enables us to support any non-http services in the future like WebSocket, MQTT and even unstandard JS runtime like JerryScript. In the future, we'd like to support everyone to generate their own V3-like SDK on top of the white-labeled client, it doesn't make much sense if we require a stream collector in the white-labeled client.

I hope my explanation makes sense to you.

AllanZhengYP · 2020-05-12T23:23:14Z

@russell-dot-js

It would be nice to add some unit tests demonstrating that it properly handles ReadableStream if implemented, and falls back to blob.)

We used string body because we simply run the test in Node runtime. It will be awesome if we ca have a webstream polyfill test.

I think you can add the test in a separate PR if you want because many Amplify users are still blocked by this issue in SDK. I'd like get this fix merged in earlier

Were those scopes accidentally committed, or is the intent of test:all to only test those two packages?

The scoped test script only runs in the 2 packages. Because the unit test in the packages are run in puppeteer, so we call the test script in the 2 packages explicitly.

Correspond to aws/aws-sdk-js-v3#1123

AllanZhengYP · 2020-05-14T20:31:23Z

@russell-dot-js
Hi, I'm working on another PR adopting your commits from this PR. I will post the PR when it's ready.

* Use web universal fetch handler and stream collector Correspond to aws/aws-sdk-js-v3#1123 * move runtime-specific stream collectors to individual request handler package

github-actions · 2021-01-20T00:40:58Z

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.

russell-dot-js force-pushed the support-all-the-browsers branch from cb7e413 to 41634df Compare April 29, 2020 03:05

russell-dot-js force-pushed the support-all-the-browsers branch from 41634df to d455ef9 Compare April 29, 2020 03:06

russell-dot-js changed the title ~~Support all the webs~~ refactor: Support all the webs Apr 29, 2020

russell-dot-js force-pushed the support-all-the-browsers branch 2 times, most recently from 0e6f077 to d638485 Compare April 29, 2020 03:23

AllanZhengYP reviewed Apr 29, 2020

View reviewed changes

AllanZhengYP requested a review from trivikr April 29, 2020 18:54

russell-dot-js force-pushed the support-all-the-browsers branch from d638485 to ba7274e Compare April 29, 2020 21:18

russell-dot-js mentioned this pull request May 6, 2020

fix: override default CognitoIdentityClient config to remove assumption of fetch body support aws-amplify/amplify-js#5707

Closed

AllanZhengYP added a commit to AllanZhengYP/smithy-typescript that referenced this pull request May 12, 2020

Use web universal fetch handler and stream collector

71a4b65

Correspond to aws/aws-sdk-js-v3#1123

AllanZhengYP suggested changes May 12, 2020

View reviewed changes

AllanZhengYP added a commit to AllanZhengYP/smithy-typescript that referenced this pull request May 12, 2020

Use web universal fetch handler and stream collector

92c1752

Correspond to aws/aws-sdk-js-v3#1123

russell-dot-js force-pushed the support-all-the-browsers branch from ba7274e to 0eedacd Compare May 12, 2020 20:58

fix: support all the webs

0bc10e8

* Collapse stream-collector-browser and stream-collector-native into stream-collector-web * Update FetchHttpHandler to return blob if stream not implemented

russell-dot-js force-pushed the support-all-the-browsers branch from 0eedacd to 0bc10e8 Compare May 12, 2020 21:00

russell-dot-js added 2 commits May 12, 2020 14:24

Merge branch 'master' of github.com:aws/aws-sdk-js-v3 into support-al…

4de99a2

…l-the-browsers

fix: check for existence of response body, not ReadableStream impl

fb381d9

russell-dot-js force-pushed the support-all-the-browsers branch from 86dd467 to fb381d9 Compare May 12, 2020 21:31

AllanZhengYP added a commit to AllanZhengYP/smithy-typescript that referenced this pull request May 14, 2020

Use web universal fetch handler and stream collector

effaaed

Correspond to aws/aws-sdk-js-v3#1123

AllanZhengYP mentioned this pull request May 14, 2020

feat: refactor http request handlers #1186

Merged

AllanZhengYP added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 15, 2020

AllanZhengYP closed this May 28, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Support all the webs #1123

refactor: Support all the webs #1123

russell-dot-js commented Apr 29, 2020 •

edited

Loading

aws-sdk-js-automation commented Apr 29, 2020

aws-sdk-js-automation commented Apr 29, 2020

russell-dot-js commented Apr 29, 2020

aws-sdk-js-automation commented Apr 29, 2020

aws-sdk-js-automation commented Apr 29, 2020

aws-sdk-js-automation commented Apr 29, 2020

AllanZhengYP left a comment

AllanZhengYP Apr 29, 2020

AllanZhengYP commented Apr 29, 2020

russell-dot-js commented Apr 29, 2020 •

edited

Loading

russell-dot-js commented Apr 29, 2020 •

edited

Loading

aws-sdk-js-automation commented Apr 29, 2020

russell-dot-js commented May 6, 2020

AllanZhengYP commented May 12, 2020

AllanZhengYP May 12, 2020

russell-dot-js May 12, 2020

codecov-io commented May 12, 2020 •

edited

Loading

aws-sdk-js-automation commented May 12, 2020

aws-sdk-js-automation commented May 12, 2020

aws-sdk-js-automation commented May 12, 2020

aws-sdk-js-automation commented May 12, 2020

AllanZhengYP commented May 12, 2020

AllanZhengYP commented May 12, 2020

AllanZhengYP commented May 14, 2020

github-actions bot commented Jan 20, 2021

refactor: Support all the webs #1123

refactor: Support all the webs #1123

Conversation

russell-dot-js commented Apr 29, 2020 • edited Loading

aws-sdk-js-automation commented Apr 29, 2020

AWS CodeBuild CI Report

aws-sdk-js-automation commented Apr 29, 2020

AWS CodeBuild CI Report

russell-dot-js commented Apr 29, 2020

aws-sdk-js-automation commented Apr 29, 2020

AWS CodeBuild CI Report

aws-sdk-js-automation commented Apr 29, 2020

AWS CodeBuild CI Report

aws-sdk-js-automation commented Apr 29, 2020

AWS CodeBuild CI Report

AllanZhengYP left a comment

Choose a reason for hiding this comment

AllanZhengYP Apr 29, 2020

Choose a reason for hiding this comment

AllanZhengYP commented Apr 29, 2020

russell-dot-js commented Apr 29, 2020 • edited Loading

russell-dot-js commented Apr 29, 2020 • edited Loading

aws-sdk-js-automation commented Apr 29, 2020

AWS CodeBuild CI Report

russell-dot-js commented May 6, 2020

AllanZhengYP commented May 12, 2020

AllanZhengYP May 12, 2020

Choose a reason for hiding this comment

russell-dot-js May 12, 2020

Choose a reason for hiding this comment

codecov-io commented May 12, 2020 • edited Loading

Codecov Report

aws-sdk-js-automation commented May 12, 2020

AWS CodeBuild CI Report

aws-sdk-js-automation commented May 12, 2020

AWS CodeBuild CI Report

aws-sdk-js-automation commented May 12, 2020

AWS CodeBuild CI Report

aws-sdk-js-automation commented May 12, 2020

AWS CodeBuild CI Report

AllanZhengYP commented May 12, 2020

AllanZhengYP commented May 12, 2020

AllanZhengYP commented May 14, 2020

github-actions bot commented Jan 20, 2021

russell-dot-js commented Apr 29, 2020 •

edited

Loading

russell-dot-js commented Apr 29, 2020 •

edited

Loading

russell-dot-js commented Apr 29, 2020 •

edited

Loading

codecov-io commented May 12, 2020 •

edited

Loading