benjreinhart / react-native-aws3

Pure JavaScript React Native library for uploading to AWS S3
MIT License
399 stars 151 forks source link

Promise flow is broken #17

Closed chadwilken closed 7 years ago

chadwilken commented 7 years ago

Currently in the Request class you have the option for a catch, the problem is that the RNS3 class creates and then immediately calls send on the XMLHTTPRequest, which if there is an issue will fail before the return, this prevents the catch method a user might add from being called.

benjreinhart commented 7 years ago

I'm not sure I'm following, wouldn't calling .catch(fn) on the return value of RNS3.put handle any rejected promises, even if they're initially rejected before the put method returns?

I would expect behavior like I see in my console:

screen shot 2016-11-03 at 5 56 02 pm

benjreinhart commented 7 years ago

Do you have any super simple reproducible cases you could share?

chadwilken commented 7 years ago

So we have

const file = {
  uri: `file://${path}`,
  name: `${photo.uuid}.jpg`,
  type: 'image/jpg',
};

const options = {
   bucket: 'our-bucket',
   region: 'us-east-1',
   accessKey: ACCESS_KEY,
   secretKey: SECRET_ACCESS_KEY,
};
RNS3.put(file, options)
  .then((response) => {
    const { postResponse } = response.body;

    if (response.status === 201) {
     // Make an api call with the returned URI
    }
  })
  .catch((error) => { console.log(error); }); // The catch doesn't get called

The catch wasn't getting called and logging to the console. I added a check to see if the device has internet above it and it seems to work now.

I think the reason that the catch doesn't get called is that when you call put it immediately opens, sets the file, and then sends the request. If the device is offline I don't think the callback gets called. Am I way off base here?

benjreinhart commented 7 years ago

Cool, I can try replicating that when I get a moment and fix it. #16 might be related too (the onerror piece).

benjreinhart commented 7 years ago

Just released a new version and I am seeing catch getting called correctly, so going to close this.