Simplified AlertIOS #5286

corbt · 2016-01-13T16:41:46Z

Ok, so this started as fixing #5273 but ended up getting a little more complicated. 😄

Currently, AlertIOS has the following API:

alert(title, message, buttons, type)
prompt(title, defaultValue, buttons, callback)

I've changed the API to look like the following:

alert(title, message, callbackOrButtons)
prompt(title, message, callbackOrButtons, type, defaultValue)

I know that breaking changes are a big deal, but I find the current alert API to be fairly inconsistent and unnecessarily confusing. I'll try to justify my changes one by one:

Currently type is an optional parameter of alert. However, the only reason to change the alert type from the default is in order to create one of the input dialogs (text, password or username/password). So we're in a weird state where if you want a normal text input, you use prompt, but if you want a password input you use alert with the 'secure-text' type. I've moved type to prompt so all text input is now done with prompt.
Currently the only way to fire a callback when a user dismisses an alert is by passing in custom buttons. However, prompt allows you to pass in a callback that is called when the user clicks the "OK" button, without having to define custom buttons. But, you have to pass null to either the callback or buttons parameter (can't define both). Since these are mutually exclusive, I've combined them into one parameter and added analogous functionality to both alert and the prompt to make their APIs more consistent.
The defaultValue functionality of prompt got broken between 0.16 and 0.17. I've added it back in as an explicit argument (the default value reused the message field before, but there are times when you might want both a default value and a message, so they should be separate).

Those were all the major changes I made. I did also refactor the UIExplorer a bit to make it more clear what's going on in each example, and updated the documentation for AlertIOS.

facebook-github-bot · 2016-01-13T16:42:48Z

By analyzing the blame information on this pull request, we identified @tadeuzagallo, @mkonicek and @christopherdro to be potential reviewers.

satya164 · 2016-01-15T02:28:55Z

I agree that current Alert API is confusing. But this is a very big breaking change, since alert is supposedly used in lots of apps. If we want to introduce a breaking change, we should provide a codemod to migrate old APIs to new one.

Also we should change the Android API as well.

Thoughts @ide @brentvatne @mkonicek ?

corbt · 2016-01-15T11:27:57Z

The only difference between this and the cross-platform Alert API is support for a callback in place of buttons. I can add that to the Android implementation though, I just figured this PR was getting big enough on its own. 😐

And actually the "breaking" part of the change is pretty minor. The most common alert call (alert(title, message, [buttons])) doesn't change at all. Also, moving the position of the prompt's defaultValue shouldn't break many peoples' apps, because defaultValue has been broken since 0.17 and no one has complained about it. 😄

Only the following usages will need to be migrated with this change:

prompt(title, message, null, callback) -> prompt(title, message, callback). Note that this doesn't make the API less configurable, just less confusing, because you weren't allowed to define both buttons and a callback anyway.
alert(title, message, buttons, type) -> prompt(title, message, buttons, type). This change may actually affect people in the wild who were using password/login input types. But as I explain above, the only reason to change the type is if you want to use one of the text input alerts. If we're going to have a separate prompt function anyway (which I think is a clearer API), these types should live on prompt.

Is there an example of a breaking change with a codemod I could examine to see how to write/integrate that? I could also shim support for the old API for one version with deprecation warnings, which people may be more familiar with.

satya164 · 2016-01-15T14:27:28Z

@corbt I've not seen many breaking changes, so no idea about it. I'm also working on #5336 , which will introduce a new API, and we need to decide a proper way of deprecating old APIs. I'm not sure what's the best way to do it though :(

facebook-github-bot · 2016-01-18T12:42:06Z

@corbt updated the pull request.

corbt · 2016-01-18T12:45:37Z

Ok I've updated this to add support for the old API with console.warn deprecation notices, so unless I've missed something this shouldn't break anyone's apps. @mkonicek anyone at FB able to take a look at this? May affect your internal apps.

mkonicek · 2016-01-18T21:54:15Z

Libraries/Utilities/Alert.js

@@ -68,10 +67,9 @@ class Alert {
    title: ?string,
    message?: ?string,
    buttons?: Buttons,
-    type?: AlertType


This was for convenience on iOS. How can people pass the type / style with this change?

The only reason to change the type is to switch from an alert to a prompt, and I think it's very unlikely that anyone was using a dialog that appears as a prompt on one platform and an alert on another. But I can add that arg back in with a deprecation warning.

mkonicek · 2016-01-18T21:58:44Z

Thanks for working on it! Yup a quick search tells me we have about a dozen files at fb that use AlertIOS.

facebook-github-bot · 2016-01-18T23:06:14Z

@corbt updated the pull request.

corbt · 2016-01-18T23:06:42Z

@mkonicek I've restored support for a type parameter in Alert.alert() with a deprecation warning.

mkonicek · 2016-01-19T19:55:33Z

Thanks for cleaning up the API! Like the new one much better.

This is now 100% backwards compatible, correct?

corbt · 2016-01-19T21:46:43Z

@mkonicek yes, with deprecation warnings.

satya164 · 2016-01-19T21:51:17Z

Idea: In places where we've deprecation warnings, we can add a comment // TODO: deprecated, which will help us find these and remove entirely later.

mkonicek · 2016-01-20T17:55:43Z

Nice! If you guys are confident this definitely doesn't break any possible existing call site feel free to comment @facebook-github-bot shipit. Hope I'm not going to have a hard time being pinged by product teams at fb telling me their alerts stopped working :)

mkonicek · 2016-01-20T17:57:21Z

Libraries/Utilities/AlertIOS.js

+   *    `style` should be one of 'default', 'cancel' or 'destructive'.
+   *  - **type**: string -- This configures the text input. One of 'plain-text',
+   *    'secure-text' or 'login-password'.
+   *  - **defaultValue**: string -- the default value for the text field.


nit: I like the emphasis for the names but not sure it's a convention in the docs. Check other files. Would rather keep it consistent with docs for other files and then migrate all at once.

facebook-github-bot · 2016-01-20T23:42:08Z

@corbt updated the pull request.

corbt · 2016-01-20T23:44:11Z

@mkonicek so as near as I can tell there isn't a great story for documenting function arguments in the RN docs right now. The only API I could find that had anything more formal than a prose definition is LayoutAnimation, which uses the @param annotation. I've updated this PR to use the same annotation. Long-term it would be good to add formatting to the docs generator to deal with @param or an equivalent keyword nicely.

mkonicek · 2016-01-21T16:39:22Z

Oh but @param is not going to render well on the website now. Can you make this just a list please?

corbt · 2016-01-21T16:51:16Z

@mkonicek this better?

facebook-github-bot · 2016-01-21T16:51:18Z

@corbt updated the pull request.

mkonicek · 2016-01-21T17:33:09Z

Yup that should render fine :) Looks good, ship it 👍

corbt · 2016-01-21T17:37:06Z

@facebook-github-bot shipit

facebook-github-bot · 2016-01-21T17:37:33Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1690217851214564/int_phab to review.

Summary: Ok, so this started as fixing facebook#5273 but ended up getting a little more complicated. 😄 Currently, AlertIOS has the following API: * `alert(title, message, buttons, type)` * `prompt(title, defaultValue, buttons, callback)` I've changed the API to look like the following: * `alert(title, message, callbackOrButtons)` * `prompt(title, message, callbackOrButtons, type, defaultValue)` I know that breaking changes are a big deal, but I find the current alert API to be fairly inconsistent and unnecessarily confusing. I'll try to justify my changes one by one: 1. Currently `type` is an optional parameter of `alert`. However, the only reason to change the alert type from the default is in order to create one of the input dialogs (text, password or username/password). So we're in a weird state where if you want a normal text input, you use `prompt`, but if you want a password input you use `alert` with the 'secure-text' type. I've moved `type` to `prompt` so all text input is now done with `pro Closes facebook#5286 Reviewed By: svcscm Differential Revision: D2850400 Pulled By: androidtrunkagent fb-gh-sync-id: 2986cfa2266225df7e4dcd703fce1e322c12b816

Noitidart · 2018-02-13T01:33:47Z

Is there a hidden AlertAndroid.prompt? I'm looking for the native prompt functionality on android. I looked inside UIManager but could not find a prompt for Android.

ansarikhurshid786 · 2020-01-27T11:28:30Z

@Noitidart I am also looking for the same. have you got solution?

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 13, 2016

corbt force-pushed the simplified-alertios branch 2 times, most recently from edc0b34 to 2e657b8 Compare January 18, 2016 12:41

mkonicek reviewed Jan 18, 2016
View reviewed changes

corbt force-pushed the simplified-alertios branch from 2e657b8 to fa7a5c0 Compare January 18, 2016 23:05

mkonicek reviewed Jan 20, 2016
View reviewed changes

corbt force-pushed the simplified-alertios branch from fa7a5c0 to 675e12f Compare January 20, 2016 23:40

corbt added 2 commits January 21, 2016 16:50

simplified AlertIOS

5e30c6d

AlertIOS docs

d0f7cfc

corbt force-pushed the simplified-alertios branch from 675e12f to d0f7cfc Compare January 21, 2016 16:51

ghost closed this in ba4101d Jan 21, 2016

corbt deleted the simplified-alertios branch February 3, 2016 12:28

corbt mentioned this pull request Feb 6, 2016

Update AlertIOS.js #5789

Closed

corbt mentioned this pull request Mar 12, 2016

AlertIOS.prompt() default values are broken #5273

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified AlertIOS #5286

Simplified AlertIOS #5286

corbt commented Jan 13, 2016

facebook-github-bot commented Jan 13, 2016

satya164 commented Jan 15, 2016

corbt commented Jan 15, 2016

satya164 commented Jan 15, 2016

facebook-github-bot commented Jan 18, 2016

corbt commented Jan 18, 2016

mkonicek Jan 18, 2016

corbt Jan 18, 2016

mkonicek commented Jan 18, 2016

facebook-github-bot commented Jan 18, 2016

corbt commented Jan 18, 2016

mkonicek commented Jan 19, 2016

corbt commented Jan 19, 2016

satya164 commented Jan 19, 2016

mkonicek commented Jan 20, 2016

mkonicek Jan 20, 2016

facebook-github-bot commented Jan 20, 2016

corbt commented Jan 20, 2016

mkonicek commented Jan 21, 2016

corbt commented Jan 21, 2016

facebook-github-bot commented Jan 21, 2016

mkonicek commented Jan 21, 2016

corbt commented Jan 21, 2016

facebook-github-bot commented Jan 21, 2016

Noitidart commented Feb 13, 2018

ansarikhurshid786 commented Jan 27, 2020

Simplified AlertIOS #5286

Simplified AlertIOS #5286

Conversation

corbt commented Jan 13, 2016

facebook-github-bot commented Jan 13, 2016

satya164 commented Jan 15, 2016

corbt commented Jan 15, 2016

satya164 commented Jan 15, 2016

facebook-github-bot commented Jan 18, 2016

corbt commented Jan 18, 2016

mkonicek Jan 18, 2016

Choose a reason for hiding this comment

corbt Jan 18, 2016

Choose a reason for hiding this comment

mkonicek commented Jan 18, 2016

facebook-github-bot commented Jan 18, 2016

corbt commented Jan 18, 2016

mkonicek commented Jan 19, 2016

corbt commented Jan 19, 2016

satya164 commented Jan 19, 2016

mkonicek commented Jan 20, 2016

mkonicek Jan 20, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Jan 20, 2016

corbt commented Jan 20, 2016

mkonicek commented Jan 21, 2016

corbt commented Jan 21, 2016

facebook-github-bot commented Jan 21, 2016

mkonicek commented Jan 21, 2016

corbt commented Jan 21, 2016

facebook-github-bot commented Jan 21, 2016

Noitidart commented Feb 13, 2018

ansarikhurshid786 commented Jan 27, 2020