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

Add Alert.alertAsync() , always returning a promise #20312

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@slorber
Contributor

slorber commented Jul 20, 2018

The point of this PR is to allow to await the completion of an Alert message

For example

const myAction = async () => {
  const choice = await Alert.alertAsync(
    'Title',
    'Message',
    [
      {text: 'Yes', onPress: () => 'yes'},
      {text: 'No', onPress: () => Promise.resolve('no')},
    ],
    {
      cancelable: true,
      onDismiss: () => 'no',
    },
  );

  if (choice === 'yes') {
    doSomething();
  }
  else {
    doSomethingElse();
  }
}

I know it's currently possible to call doSomething / doSomethingElse inside the callbacks directly, but I clearly find this more convenient to decouple the alert question asking from the side effect of this choice, and this also makes it more easy to wire an alert message in the middle of an existing async flow, and make sure this async flow still returns a promise like it used to...

Test Plan:

The current Alert JS code is not tested.

I've been using this async wrapper in my app with great success so far.

I'm willing to write tests but not really sure I'm supposed to mock the Alert.alert static call with Jest, any help on this would be welcome. Should I do Alert.alert = jest.fn(); and see how it gets called? Or is there a better way with Jest to mock this class internal static call without modifying current implementation?

Release Notes:

[GENERAL] [ENHANCEMENT] [Alert] - Add Alert.alertAsync, returning a promise

@vonovak

This comment has been minimized.

Contributor

vonovak commented Aug 7, 2018

This looks nice, but I have some doubt if it belongs to the core... Personally I find it more fitting to publish this as an independent module.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Aug 19, 2018

@slorber I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@slorber

This comment has been minimized.

Contributor

slorber commented Aug 19, 2018

Don't know who to ping about that or weither it should be core or not. I prefer promises to callbacks but it's true RN doesn't have much promise based interfaces currently. @kelset any opinion?

@slorber

This comment has been minimized.

Contributor

slorber commented Nov 13, 2018

Just to mention there is traction for this feature according to StackOverflow question views: https://stackoverflow.com/questions/48809762/how-to-await-for-the-response-of-alert-dialog-in-react-native

Can this be merged/reviewed? Should I publish this as a separate module?

@slorber

This comment has been minimized.

Contributor

slorber commented Nov 19, 2018

Hey,

Just wanted to say that I've published a package to do that: https://github.com/slorber/react-native-alert-async

If ever you want to merge this, please tell me, I'll rebase and add a fix for a bug I found in initial code ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment