-
Notifications
You must be signed in to change notification settings - Fork 587
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: refactor http request handlers #1186
feat: refactor http request handlers #1186
Conversation
* Collapse stream-collector-browser and stream-collector-native into stream-collector-web * Update FetchHttpHandler to return blob if stream not implemented
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
✅integration test:
|
Codecov Report
@@ Coverage Diff @@
## master #1186 +/- ##
==========================================
- Coverage 73.33% 73.09% -0.24%
==========================================
Files 285 286 +1
Lines 12399 12408 +9
Branches 2892 2898 +6
==========================================
- Hits 9093 9070 -23
- Misses 3306 3338 +32
Continue to review full report at Codecov.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one nit to remove console.log used while debugging
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤑 🤑 🤑
Thanks for this @AllanFly120 @russell-dot-js ! |
@AllanFly120 now that this is merged, how do we get a new release cut? |
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. |
Replaces: #1123
Resolves: #1107
Changes are made in the PR:
Previously, browsers'
fetch-http-handler
relies on the assumption thatResponse.body
is aReadableStream
. This is not true for older desktop and mobile browsers. The previous design partially addresses this issue by explicitly notifyfetch-http-handler
to return body payload inblob
instead ofReadableStream
in ReactNative whereReadableStream
is not available. But this solution doesn't solve proplems in old desktop browsers and IE.In this PR, the
fetch-http-handler
will dynamicly return body payload inblob
orReadableStream
base on whetherResponse.body
is implemented. This solution return streaming payload data when it's available. It's also flexible as it will return streaming data if user supply a web stream polyfill.Because we will use universal fetch handler in both browsers and ReactNative, we also merge the
stream-collector-browser
andstream-collector-native
into a singlestream-collector-web
, which will collect the stream if payload's streaming, or convert the payload blob to binary data. This package is merged intofetch-http-handler
. See next point for more information.According to refactor: Support all the webs #1123 (comment), I move the
stream-collector-web
intofetch-http-handler
and movestream-collector-node
intonode-http-handler
because stream collector and request handler are intertwined with each other. It would prevent people incorrectly use node stream collector with browser request handler. Moreover, the 2 components will live in the same module but are different interfaces. So we can still introduce non-http handler without a stream collector in the future/cc @russell-dot-js
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.