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.

Sign up for GitHub

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

Return promise in payIOUReport / Fix AdditionalDetailsStep #6992

Merged
merged 5 commits into from
Jan 4, 2022
Merged

Return promise in payIOUReport / Fix AdditionalDetailsStep #6992

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce1f80f
Return promise in payIOUReport
marcaaron Jan 3, 2022
3e6fd9d
fix AdditionalDetailsStep address input
marcaaron Jan 3, 2022
5fabcd8
Fix promise returned so second finally does not happen before first
marcaaron Jan 4, 2022
747bc3a
Merge remote-tracking branch 'origin' into marcaaron-improveSendMoney…
marcaaron Jan 4, 2022
8b38d0a
Merge remote-tracking branch 'origin' into marcaaron-improveSendMoney…
marcaaron Jan 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/libs/actions/IOU.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ function buildPayPalPaymentUrl(amount, submitterPayPalMeAddress, currency) {
* @param {String} [params.requestorPhoneNumber] - used for Venmo
* @param {String} [params.requestorPayPalMeAddress]
* @param {String} [params.newIOUReportDetails] - Extra details required only for send money flow
* @param {Boolean} [params.shouldRedirectToChatReport]
*
* @return {Promise}
*/
function payIOUReport({
chatReportID,
Expand All @@ -260,7 +261,6 @@ function payIOUReport({
requestorPhoneNumber,
requestorPayPalMeAddress,
newIOUReportDetails,
shouldRedirectToChatReport = false,
}) {
Onyx.merge(ONYXKEYS.IOU, {loading: true, error: false});

Expand Down Expand Up @@ -304,12 +304,9 @@ function payIOUReport({
})
.finally(() => {
Onyx.merge(ONYXKEYS.IOU, {loading: false});

if (shouldRedirectToChatReport) {
Navigation.navigate(ROUTES.REPORT);
}
}),
url);
return payIOUPromise;
Copy link
Contributor

@aldo-expensify aldo-expensify Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this could cause a bug. You are returning the original promise, not the one that gets created after chaining then-catch-finally, which you later use to finally(() => Navigation.navigate(ROUTES.REPORT)).

In the original code, the Navigation.navigate(ROUTES.REPORT); happens inside the finally of the new promise object that gets created after chaining then and catch. The current code may make your new finally run before the finally that was chained after the then and catch. To see this, run these in the console:

Case 1 (similar to PR code):

payIOUPromise = new Promise((resolve, reject) => {
  setTimeout(() => {
    resolve('foo');
  }, 300);
});

payIOUPromise
  .then(response => {
    console.log('Then:', response);
  })
  .catch(error => {
    console.log('ERROR:', error);
  })
  .finally(() => {
    console.log('Finally 1');
  });

payIOUPromise.finally(() => {
  console.log('Finally 2')
});

Output (reversed "Finally X"):

Then: foo
Finally 2
Finally 1

Case 2

payIOUPromise = new Promise((resolve, reject) => {
  setTimeout(() => {
    resolve('foo');
  }, 300);
});

payIOUPromise = payIOUPromise // < -- The difference
  .then(response => {
    console.log('Then:', response);
    return response; // Useful in this case if someone wants to do a .then outside
  })
  .catch(error => {
    console.log('ERROR:', error);
  })
  .finally(() => {
    console.log('Finally 1');
  });

payIOUPromise.finally(() => {
  console.log('Finally 2')
});

Output:

Then: foo
Finally 1
Finally 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure I'm following the logic here... the claim is that there are two different promises: one being returned from the method and another one that handles the .then() and .catch()? The observation of the difference in order seems correct, but not sure about the explanation. How is the returned promise different from the one passed to asyncOpenURL()?

For sure open to thoughts on how this change might cause a bug if you have ideas or alternatives. I couldn't think of anything. The only thing we do in the first finally() is set loading to false and navigating back to the report has no dependency on each other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only scenario I think this would cause a bug is if you wanted to make some other type of API call inside the inner catch and make sure that call completes before the outer finally, but agreed with this specific use case the difference in order doesn't seem to affect the desired behavior. That being said, my expectation here would be that the inner promise handling happens before the outer handling, so I'd prefer to have it updated so there isn't any confusion down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the returned promise different from the one passed to asyncOpenURL()?

Just answering my own question here, but after looking at this for a while I learned something new - which is that a promise will only be handled if you chain off of the result of handling it and not the first promise set up. So code like this is fine:

promise = Promise.reject();

promise.catch(() => {
    console.log('caught');
});

But if you do this...

promise = Promise.reject()

promise.catch(() => {
    console.log('caught');
});

promise.then(() => {
    console.log('I will never run');
});

you'll get an unhandled promise rejection notice 🤯

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'll get an unhandled promise rejection notice 🤯

🤯

Playing with your example, this other thing was unexpected to me, I thought that reversing the order of chained catch and then didn't matter:

but reversing the callbacks:

Promise.reject()
  .catch(() => {
    console.log('caught');
  })
  .then(() => {
    console.log('I will never run');
  })

prints

caught
I will never run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧙

}

export {
Expand Down
18 changes: 11 additions & 7 deletions src/pages/EnablePayments/AdditionalDetailsStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,13 +214,17 @@ class AdditionalDetailsStep extends React.Component {
<AddressSearch
label={this.props.translate(this.fieldNameTranslationKeys.addressStreet)}
value={this.state.addressStreet}
onChangeText={(fieldName, value) => {
if (fieldName === 'addressZipCode') {
this.clearErrorAndSetValue('addressZip', value);
return;
}

this.clearErrorAndSetValue(fieldName, value);
onChange={(values) => {
const renamedFields = {
street: 'addressStreet',
state: 'addressState',
zipCode: 'addressZip',
city: 'addressCity',
};
_.each(values, (value, inputKey) => {
const renamedInputKey = lodashGet(renamedFields, inputKey, inputKey);
this.clearErrorAndSetValue(renamedInputKey, value);
});
}}
errorText={this.getErrorText('addressStreet')}
/>
Expand Down
6 changes: 4 additions & 2 deletions src/pages/iou/IOUModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,10 @@ class IOUModal extends Component {
requestorPhoneNumber: this.state.participants[0].phoneNumber,
comment,
newIOUReportDetails,
shouldRedirectToChatReport: true,
});
})
.finally(() => {
Navigation.navigate(ROUTES.REPORT);
});
}

/**
Expand Down