feat(s3): custom xhr transfer handler #11471
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## v5/custom-clients #11471 +/- ##
====================================================
Coverage ? 83.33%
====================================================
Files ? 276
Lines ? 20653
Branches ? 4459
====================================================
Hits ? 17212
Misses ? 3153
Partials ? 288
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Do you have bundle size metrics for this? |
|
|
||
| // Handle browser request cancellation (as opposed to a manual cancellation) | ||
| xhr.addEventListener('abort', () => { | ||
| if (!xhr) return; |
There was a problem hiding this comment.
If it's possible to fire the event listener while the reference to xhr is nulled out, does that mean there's a memory leak?
There was a problem hiding this comment.
Axios does the same to clean up the xhr instance. It's not for the memory leak, but more of for unexpected event listener invocations. For example, more data coming in when the abort is already called by server.
There was a problem hiding this comment.
Weird. If the event listeners registered against an instance of xhr can still fire even after the reference to it is set to null.. Doesn't that mean that listener is technically now a memory leak? What happens to that listener when I point xhr at a new instance again and add listeners to that? 🤔
| // TODO: V6 use xhr.onloadend() when we officially drop support for IE11. Keep it to reduce surprise even though we | ||
| // don't support IE11 in v5. | ||
| setTimeout(onloadend); |
There was a problem hiding this comment.
I am not sure we need this
| ResponseBodyMixin, | ||
| } from '@aws-amplify/core/internals/aws-client-utils'; | ||
| import { ConsoleLogger as Logger } from '@aws-amplify/core'; | ||
| import type * as events from 'events'; |
There was a problem hiding this comment.
@elorzafe It's for typing the emitter, which will listen to the upload/download progress events
There was a problem hiding this comment.
Why using emitter and not Hub?
There was a problem hiding this comment.
It's a parity with current Axios handler to reduce future refactor to S3 provider
AllanZhengYP
force-pushed
the
v5/custom-clients-s3-merge-xhr
branch
from
a31bdef
to
b09af04
Compare
* feat(s3): implement an XHR transfer handler to replace Axios * fix(s3): address feedbacks
Description of changes
An XHR based HTTP transfer handler to be used by custom S3 client. This is to replace the current Axios based handler injected to AWS SDK S3 client. Like Axios handler, this handler also supports abort and download & upload progress events. The thrown error and events to intentionally compatible with Axios ones.
This xhr handler is 1/4 the size of Axios handler
Description of how you validated changes
Manually tested on React Native, browsers.
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.