packages/stream-collector-browser not compatible with Edge/IE11/Older versions of android

[russell-dot-js]

Confirm by changing [ ] to [x] below to ensure that it's a bug:

• I've gone through Developer Guide and API reference
• I've checked AWS Forums and StackOverflow for answers
• I've searched for previous similar issues and didn't find any solution

Describe the bug
packages/stream-collector-browser exports the following function:

export const streamCollector: StreamCollector = (
  stream: ReadableStream
): Promise<Uint8Array> => {
  return new Response(stream)
    .arrayBuffer()
    .then(arrayBuffer => new Uint8Array(arrayBuffer));
};

However, the constructor Response(ReadableStream), as defined in Typescripts lib.dom, is not implemented in IE, Edge, older versions of android, and (potentially - unknown) other older browsers. The code results in error "invalid arguments".

Compounding the issue is that since fetch itself is implemented in some of these browsers, polyfills such as whatwg-fetch will not be applied.

Is the issue in the browser/Node.js?
Browser

Details of the browser/Node.js version
Microsoft Edge 44.17763.1.0
IE11

SDK version number
1.0.0-beta.3

To Reproduce (observed behavior)
Easy: run any app using cognito identity with the @aws-amplify library in IE11, Edge, or Older android versions (the official Microsoft VM reproduces just fine: https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/)

Once logged in, stream-collector-browser's attempt to turn the cached Cognito identity will fail, resulting in subsequent requests being signed with accessKeyId "undefined", despite successful authentication with Cognito.

Expected behavior
streamCollector() should not throw an error

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Related issues:
aws-amplify/amplify-js#5332
aws-amplify/amplify-js#5534

I've spent a couple days debugging this issue, and working on potential fixes. Trying to ponyfill Response with whatwg-fetch or node-fetch's Response has not yet been successful - node-fetch works in Chrome but Edge is still broken, whatwg-fetch actually causes Chrome to start throwing the same error that Edge already throws (digging in, it looks like whatwg-fetch does not properly implement the Response(ReadableStream) constructor.

Suggestion: I am not wildly familiar with these new API's and didn't find an easy way to convert a ReadableStream to a Uint8Array. Buffer.from(String(stream)); seems like the easy way to do it, but does not seem to work (I'm aware this is for node-only). My concern is that this new aws sdk, which hopefully we can all use in production soon, requires fetch to be properly implemented globally. Sadly, fetch is not part of the ECMA spec, and cannot safely be used in production today without being transpiled.

[russell-dot-js]

@AllanFly120 tagging you as you have recently contributed to packages/stream-collector-browser

[russell-dot-js]

More information: updating runtimeconfig.browser.ts in client-cognito-identity with the following makes amplify work in both chrome and edge/ie:

--- a/clients/client-cognito-identity/runtimeConfig.browser.ts
+++ b/clients/client-cognito-identity/runtimeConfig.browser.ts
@@ -1,8 +1,8 @@
 import { name, version } from "./package.json";
 import { Sha256 } from "@aws-crypto/sha256-browser";
-import { FetchHttpHandler } from "@aws-sdk/fetch-http-handler";
+import { NodeHttpHandler } from "@aws-sdk/node-http-handler/build/node-http-handler";
 import { invalidFunction } from "@aws-sdk/invalid-dependency";
-import { streamCollector } from "@aws-sdk/stream-collector-browser";
+import { streamCollector } from "@aws-sdk/stream-collector-node";

@@ -20,7 +20,7 @@ export const ClientDefaultValues: Required<ClientDefaults> = {
   credentialDefaultProvider: (() => {}) as any,
   defaultUserAgent: defaultUserAgent(name, version),
   regionDefaultProvider: invalidFunction("Region is missing") as any,
-  requestHandler: new FetchHttpHandler(),
+  requestHandler: new NodeHttpHandler(),
   sha256: Sha256,
   streamCollector,
   urlParser: parseUrl,

Another note/issue: importing @aws-sdk/node-http-handler rather than @aws-sdk/node-http-handler/build/node-http-handler does not work, as @aws-sdk/node-http-handler pulls in node-http2-handler, which depends on constants exposed in "https2" from @types/node, but @types/node is listed as a devDependency.

Obviously this is a hack, but by removing our dependency on fetch (which, IMO, is poorly defined in Typescript's lib.dom - lib.dom should not define that the Response constructor can take in a ReadableStream, as that is experimental), we have a library that works in all browsers.

Suggested fixes:

• Should we remove dependency on fetch (and Response) all together, like the node implementation?
• Should we use a ponyfill instead of expecting browsers to have a fetch implementation that supports experimental features?

[russell-dot-js]

@trivikr pinging you also since you reviewed #721, which seemed to introduce the issue

Using ReadableStream should be fine as there are polyfills, but this line is problematic:
https://github.com/aws/aws-sdk-js-v3/pull/721/files#diff-e1fe01590be4f43565594ecf02c1a15eR6

[Amplifiyer]

Could be related as well aws-amplify/amplify-js#5405

[russell-dot-js]

@AllanFly120 @trivikr @Amplifiyer PR is here:
https://github.com/aws/aws-sdk-js-v3/pull/1121/files

Tested in Chrome, IE11, and Edge

[AllanZhengYP]

@russell-dot-js

Thanks a lot for the detailed analysis and description, also the PR, I will review it soon.

Is the Response constructor the only known blocker to IE11? Do you need to provide other pollyfills in order to run Amplify in IE11? As of now, we haven't finalize the decision for browser support, as a result, we don't run the test over IE.

Regarding the browser support I need to discuss with the team together with feedbacks from Amplify. Potentially, we can either remove non-compatible components(if there's more), or provide a guide to bundle the SDK for IE.

[russell-dot-js]

@russell-dot-js

Thanks a lot for the detailed analysis and description, also the PR, I will review it soon.

Is the Response constructor the only known blocker to IE11? Do you need to provide other pollyfills in order to run Amplify in IE11? As of now, we haven't finalize the decision for browser support, as a result, we don't run the test over IE.

Regarding the browser support I need to discuss with the team together with feedbacks from Amplify. Potentially, we can either remove non-compatible components(if there's more), or provide a guide to bundle the SDK for IE.

Polyfills are still required for Web Streams and fetch, but that's pretty much BAU for front-end orgs. The big issue here is that you can't polyfill to overload a constructor AFAIK, hence why the Response() constructor was replaced with another implementation

[russell-dot-js]

I did some more tests to find a production-ready usage, and found the following issues with the popular polyfills:

• isomorphic-fetch pulls in an older version of whatwg-fetch, which does not implement iterable Headers (error: "Object is not iterable").
• The most popular client-side polyfill, whatwg-fetch, does not support returning body as a ReadableStream (IE error: "Unable to get property 'getReader' of undefined or null reference"). whatwg-fetch is considered "completed" and will not be updated to support streams. (see this and this)

After cycling through various polyfills, @stardazed/streams-polyfill works excellently - and even monkey-patches the Response constructor so that the sdk works in-browser without my fix, but I'm curious if there's more we should do to make the sdk compatible with different environments.

• Should we give up on expecting fetch to work cross-environment, and pull in axios or node-fetch instead?
• Should we keep current fetch implementation, but provide a clear example for how to override with the middleware/config model (e.g. if amplify-js wants to use axios to be browser-safe without polyfills, how could they do that?). This would allow putting the responsibility of compatibility on the consumer, while keeping the SDK light and extensible.

@AllanFly120 and @trivikr - given the vision for the new sdk, where should we take it from here?

@Amplifiyer - what is the correct guidance for the amplify community? Should amplify include a polyfill, suggest users pull in the stardazed polyfill, or ... ?

[AllanZhengYP]

Hey @russell-dot-js

I think the main problem to solve here is the SDK's dependency on ReadableStream body from the fetch response. By default the SDK assumes response.body is a stream. if the response is streaming(e.g. in S3.getObject), SDK will return this stream from low-level fetch handler directly to users. Whereas for other 'normal' APIs, SDK will collect the stream back to buffer and parse it.

This structure brings us the problem that if users need to polyfill the fetch, the response.body is usually not implemented perfectly(e.g. github/fetch doesn't implement stream). It results in SDK cannot read the body even if users polyfill the fetch. So in this case, the SDK cannot get the data from response by default. However, we already solved this problem in ReactNative, where readableStream is not available either. In ReactNative, we don't support streaming response, instead, we always return a blob for response body.

If you want to use the same solution in your code, you can:

import {FooClient} from "@aws-sdk/client-foo";
import {FetchHttpHandler} from "@aws-sdk/fetch-http-handler";
import { streamCollector } from "@aws-sdk/stream-collector-native";
const client = new FooClient({
    requestHandler: FetchHttpHandler({bufferBody: true}),
    streamCollector // this always pairs with 'requestHandler'
})

With this change, you can polyfill the SDK with pretty much all the providers you mentioned above. But I know it requires code change, this definitele does not fix issue for most prod apps. I will dig more for solutions not requiring code changes.

[AllanZhengYP]

One solution on the top of my head is that inside the fetch handler, we return the body in different shapes basing on whether ReadableStream is available.

if (typeof ReadableStream === 'function' && response.body instanceof ReadableStream)  {
    // return the response body as a ReadableStream
} else {
    // return response body as blob
    return response.blob().then(body => ({
        response: new HttpResponse({
          headers: transformedHeaders,
          statusCode: response.status,
          body
        })
      }));
}

In this case the SDK will return a ReadableStream body when runtime supports it, and a blob otherwise. This good thing is that all most all the fetch polyfills support response.blob() so the SDK will work with those polyfills.

[russell-dot-js]

@AllanFly120 very cool that requestHandler and streamCollector can already be overridden by calling the constructor yourself. I was only looking at ClientDefaultValues, not the constructor, and was going to suggest this change, but it looks like it already works the way I was hoping.

That being said, the decision is whether or not the sdk should try to support every environment, or whether that responsibility falls on the consumer. I'm more than happy to put up a PR that implements fetch-http-handler and stream-collector-browser to work whether or not fetch (and the Response constructor) are implemented, but before going down that path, I want to make sure that the intention of this sdk is to support both cases, or if we should put that responsibility on the consumer.

[russell-dot-js]

@AllanFly120 here you go, I was bored:
https://github.com/aws/aws-sdk-js-v3/pull/1123/files

[lock]

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.