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

S3.putObject doesn't support non-node ReadableStream #2202

Closed
reaperdtme opened this issue Aug 15, 2018 · 8 comments
Closed

S3.putObject doesn't support non-node ReadableStream #2202

reaperdtme opened this issue Aug 15, 2018 · 8 comments
Assignees
Labels
closed-for-staleness documentation This is a problem with documentation.

Comments

@reaperdtme
Copy link

S3 docs claim that S3.putObject supports ReadableStream

Body — (Buffer, Typed Array, Blob, String, ReadableStream)
Object data.

But when used in amplify-js in a React Native app, Readable Stream is not supported.

StorageClass - error uploading: Error: Unsupported body payload object
    at ManagedUpload.self.fillQueue

Currently, streams are only supported in NodeJS environments.

https://github.com/aws/aws-sdk-js/blob/f7728f45dd80c5d4919652720a773d343aaab8af/lib/s3/managed_upload.js#L178

if (self.sliceFn) {
      self.fillQueue = self.fillBuffer;
    } else if (AWS.util.isNode()) {
      var Stream = AWS.util.stream.Stream;
      if (self.body instanceof Stream) {

Please expand stream support for ReadableStream as defined by:
https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream

Or provide a general definition or interface of ReadableStream.

Feature necessary since streams keep IO at low memory footprint. Currently, loading entire HD videos into memory for an upload is significantly suboptimal.

@chrisradek
Copy link
Contributor

@KidCosmic
This documentation was written long before browsers supported streams, and we likely won't be able to support them in browsers until a major version bump when we can look at using fetch instead of XmlHttpRequest. We can definitely update our documentation to make it clear that this only works with node.js streams today.

Are you able to use Blobs? I'm not sure how they differ in react native versus in the browsers, but if they work like the File object in browsers, that may be an alternative way to upload large files.

I didn't know react native supported ReadableStream with fetch. Is is officially supported, or provided through a 3rd party package?

@chrisradek chrisradek added the documentation This is a problem with documentation. label Aug 15, 2018
@reaperdtme
Copy link
Author

reaperdtme commented Aug 16, 2018

ReadableStream is actually supported by the JavascriptCore, ie, Safari. We're actually shaping another stream into ReadableStream via the ReadableStream constructor:

function convertStreamToReadableStream(_in) {
  return new ReadableStream({
    start(controller) {
      _in.open()
      _in.onData((chunk) => {
        controller.enqueue(chunk)
      })
      _in.onError((error) => {
        controller.error(error)
      })
      _in.onEnd(() => {
        controller.close()
      })
    }
  })
}

Doesn't even look like even fetch accepts streams as body arguments, which kinda makes this whole discussion moot, lmao.

I'll look at File/Blob but I can't seem to get blobs outside of blobbing a response body. Their constructors are out of the question because of loading into mem. Hmmm... Will most likely have to native this

@chrisradek
Copy link
Contributor

You know, we should actually be able to update our ManagedUpload class (used by s3.upload) so that it pulls data from the stream using a reader. The upload method performs a multi-part upload if the file is larger than 5 MB, so it would essentially read up to 5 MB from the stream at a time for each uploadPart call. We wouldn't be able to make this work for putObject. Likewise you should be able to pull the same thing off calling the multipart upload APIs.

You're right, I was only able to use a ReadableStream with fetch if it originally came from a separate fetch response in (I think) chrome. That was a while ago, but was hoping that story had improved.

@chrisradek chrisradek added the feature-request A feature should be added or improved. label Aug 16, 2018
@reaperdtme
Copy link
Author

reaperdtme commented Aug 16, 2018

That's pretty clever actually. Sounds like it would work to me. 5MB in memory much better than 20

So per the blob suggestion, the pattern works in React Native. XMLHttpRequest accepts a plain js object with {uri, optional field, optional name}, and XHR does the dirty work under the hood. Note that both XHR.send and FormData.add accept this. For example:
facebook/react-native#15724

There is no explicit formalization of it, just that it works.

Possible solution would be in ManagedUpload
if isReactNative() acceptUri
like
if isNode() acceptStream, but that's a whole do we support X debate.

Would love to use Storage with amplifyjs, but it looks we'd probably have to drop down to aws-sdk-js and run with getSignedUrl if we want XHR.sendUri.

Could also take this to Amplify team and have them implement getSignedUrl in the case of {uri} since they seem to explicitly support react native.

ReadableStream definitely on your turf though..

@srchase srchase removed the feature-request A feature should be added or improved. label Jan 17, 2019
@ngocketit
Copy link

@KidCosmic Could you please share some code of your solution? I'm also running into the same problem trying to upload a large file using stream.

@reaperdtme
Copy link
Author

@ngocketit went with presigned urls. Ask your server for a signed s3 url and do a simple XMLHttpRequest like this:

function sendFile(
  presignedurl: string,
  file: { uri: string; type?: string; name?: string },
  onSuccess: () => void,
  onFail: () => void
) {
  // from http://blog.rudikovac.com/react-native-upload-any-file-to-s3-with-a-presigned-url/
  const xhr = new XMLHttpRequest();
  xhr.onreadystatechange = function() {
    if (xhr.readyState === 4) {
      if (xhr.status === 200) {
        // Successfully uploaded the file.
        console.log("successfully uploaded presignedurl");
        console.log(xhr);
        onSuccess();
      } else {
        console.log("xhr status");
        // The file could not be uploaded.
        console.log("failed to upload presignedurl");
        console.log(xhr);
        onFail();
      }
    }
  };
  xhr.onerror = err => {
    console.error("failed upload", err);
    onFail();
  };
  xhr.open("PUT", presignedurl);
  // for text file: text/plain, for binary file: application/octet-stream
  xhr.setRequestHeader("Content-Type", file.type || "application/octet-stream");
  xhr.send(file);
}

@ngocketit
Copy link

@KidCosmic Thanks for that. It worked!

@github-actions
Copy link

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Apr 29, 2021
@github-actions github-actions bot closed this as completed May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness documentation This is a problem with documentation.
Projects
None yet
Development

No branches or pull requests

5 participants