Skip to content

Deprecate AlertMacOS in favor of Alert #662

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

Closed
wants to merge 5 commits into from
Closed

Deprecate AlertMacOS in favor of Alert #662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5723c1
address feedback; fix iOS build
thymikee Dec 2, 2020
27ebe50
Deprecate AlertMacOS in favor of Alert
thymikee Dec 2, 2020
3d87b36
move UIAlertController define to RCTAlertController.h
thymikee Dec 3, 2020
1cb846e
define only on macos target
thymikee Dec 3, 2020
8973d60
revert podfile.lock
thymikee Dec 14, 2020
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
68 changes: 62 additions & 6 deletions Libraries/Alert/Alert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

'use strict';

import AlertMacOS from './AlertMacOS'; // TODO(macOS ISS#2323203)
import {type DefaultInputsArray} from './AlertMacOS'; // TODO(macOS ISS#2323203)
import Platform from '../Utilities/Platform';
import NativeDialogManagerAndroid, {
type DialogOptions,
Expand All @@ -33,6 +33,10 @@ export type Buttons = Array<{
type Options = {
cancelable?: ?boolean,
onDismiss?: ?() => void,
// [TODO(macOS ISS#2323203)
modal?: ?boolean,
critical?: ?boolean,
// ]TODO(macOS ISS#2323203)
...
};

Expand All @@ -48,11 +52,20 @@ class Alert {
buttons?: Buttons,
options?: Options,
): void {
if (
Platform.OS === 'ios' ||
Platform.OS === 'macos' /* TODO(macOS ISS#2323203) */
) {
if (Platform.OS === 'ios') {
Alert.prompt(title, message, buttons, 'default');
// [TODO(macOS ISS#2323203)
} else if (Platform.OS === 'macos') {
promptMacOS(
title,
message,
buttons,
'default',
undefined,
options?.modal,
options?.critical,
);
// ]TODO(macOS ISS#2323203)
} else if (Platform.OS === 'android') {
if (!NativeDialogManagerAndroid) {
return;
Expand Down Expand Up @@ -156,10 +169,53 @@ class Alert {
// [TODO(macOS ISS#2323203)
} else if (Platform.OS === 'macos') {
const defaultInputs = [{default: defaultValue}];
AlertMacOS.prompt(title, message, callbackOrButtons, type, defaultInputs);
promptMacOS(title, message, callbackOrButtons, type, defaultInputs);
Copy link
Author

Choose a reason for hiding this comment

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

Didn't add modal and critical arguments here, as the original implementation didn't have that. However, I can see that it may be necessary to add in the future, which would change the prompt function signature. Ideas?

Copy link
Member

Choose a reason for hiding this comment

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

@HeyImChris @tom-un @NickGerleman How have you historically dealt with such deviating decision making?

Copy link
Author

Choose a reason for hiding this comment

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

Long-term I think it would be best to refactor the prompt in RN core to have the same API as alert. This way we could fit all extra platform-specific options into the last options argument. It be a breaking change, but I may be worth it. Should also be pretty simple to write a codemod, although I have no experience in that yet (would be happy to contribute). If that makes sense to you, I can tag Eli from the RN core and see what's his thoughts

Copy link

Choose a reason for hiding this comment

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

There are a whole bunch of different solutions to public API deviation. The north star is to unify core APIs to be more desktop friendly. This is definitely the hardest option from both technical and non technical perspectives though. I've historically pushed pretty aggressively for doing this, as it saves a lot of pain in the long run, can benefit a greater ecosystem, and tends to produce higher quality changes.

For smaller components/Libares, something like AlertMacOS is not a particularly bad option. The benefit of having a separate component is that you can provide custom typings easily, and are insulated from upstream refactorings that tend to break forking in high traffic areas. Downside is you don't get upstream refactorings as well, and consumption is annoying.

The really problematic cases I've seen have been for adding extra parameters to public methods, or extra methods/props. The story for discoverability and documentation is bad, and needing usage to be untyped has been really problematic for us.

I think this is more problematic for a team like Office though, where we simultaneously want to upgrade many projects across many repos to a new version of React Native at once. That sort of thing becomes really risky without good typings.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @NickGerleman. This basically sums up my struggle with this issue, involving API decisions and balancing the ease of keeping the fork up to date.

I could throw a proposal on D&P repo to change the prompt API to be somewhat more generic:

-static prompt(title, message?, callbackOrButtons?, type?, defaultValue?, keyboardType?)
+static prompt(title, message?, callbackOrButtons?, options?)

which would allow to put android/ios/macos/windows specific options there. As you mentioned, this approach leads to less type safe code, since one could omit props that's specific for certain platform or pass the ones for another platform, thinking it works for theirs as well. Naming conventions like prefixing with macos_ would help a bit.

Otherwise the sane decision would be to stay with AlertMacOS (or Alert.macos.js with an API of AlertMacOS), which provides the best type safety, but diverges from upstream implementation. However, since the issue was about deprecating AlertMacOS and going with Alert, I tried to figure out what's possible without it.

Let me know how and if we should proceed further :)

Copy link

Choose a reason for hiding this comment

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

This is a really good question and something that I've struggled with before. Maybe I'm in the minority here but I find that forcing platforms to conform to an API set that doesn't make sense for the platform to not be worth the benefits of having one API.

Having "options" at the end is a good solution if we needed to keep the API identical, but I'm not convinced we need to do that. We can still #ifdef and share code between iOS/macOS when possible and also have reasonable APIs that make sense for the platforms they're on.

I always have to remind myself that RN isn't "write once, run anywhere", it's "learn once, write anywhere" so it's okay for platforms APIs to sometimes vary when it makes sense.

}
// ]TODO(macOS ISS#2323203)
}
}

// [TODO(macOS ISS#2323203)
function promptMacOS(
title: ?string,
message?: ?string,
callbackOrButtons?: ?((text: string) => void) | Buttons,
type?: ?AlertType = 'plain-text',
defaultInputs?: DefaultInputsArray,
modal?: ?boolean,
critical?: ?boolean,
): void {
let callbacks = [];
const buttons = [];
if (typeof callbackOrButtons === 'function') {
callbacks = [callbackOrButtons];
} else if (callbackOrButtons instanceof Array) {
callbackOrButtons.forEach((btn, index) => {
callbacks[index] = btn.onPress;
if (btn.text || index < (callbackOrButtons || []).length - 1) {
const btnDef = {};
btnDef[index] = btn.text || '';
buttons.push(btnDef);
}
});
}

RCTAlertManager.alertWithArgs(
{
title: title || undefined,
message: message || undefined,
buttons,
type: type || undefined,
defaultInputs,
modal: modal || undefined,
critical: critical || undefined,
},
(id, value) => {
const cb = callbacks[id];
cb && cb(value);
},
);
}
// ]TODO(macOS ISS#2323203)

module.exports = Alert;
6 changes: 6 additions & 0 deletions Libraries/Alert/AlertMacOS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
'use strict';

import type {AlertType, AlertButtonStyle} from './Alert';
import warnOnce from '../Utilities/warnOnce';

var RCTAlertManager = require('../BatchedBridge/NativeModules').AlertManager;

Expand Down Expand Up @@ -176,6 +177,11 @@ class AlertMacOS {
modal?: ?boolean,
critical?: ?boolean,
): void {
warnOnce(
'deprecated-AlertMacOS',
'"AlertMacOS" module has been deprecated in favor of "Alert" and will be removed in a future version of react-native-macos',
);

var callbacks = [];
var buttons = [];
if (typeof callbackOrButtons === 'function') {
Expand Down
6 changes: 6 additions & 0 deletions Libraries/Alert/NativeAlertManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import type {TurboModule} from '../TurboModule/RCTExport';
import * as TurboModuleRegistry from '../TurboModule/TurboModuleRegistry';
import type {DefaultInputsArray} from './AlertMacOS'; // TODO(macOS ISS#2323203)

export type Args = {|
title?: string,
Expand All @@ -22,6 +23,11 @@ export type Args = {|
cancelButtonKey?: string,
destructiveButtonKey?: string,
keyboardType?: string,
// [TODO(macOS ISS#2323203)
defaultInputs?: DefaultInputsArray,
modal?: ?boolean,
critical?: ?boolean,
Copy link

Choose a reason for hiding this comment

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

The macOS Alert styles are critical, informational, and warning so I don't think toggling on a "critical" as a bool correctly encapsulates the three options available

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK Flow has no enums, so then this should probably be a string union?

Copy link
Author

Choose a reason for hiding this comment

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

Would require changes to the RCTAlertManager.mm, as it currently takes critical as a boolean.

// ]TODO(macOS ISS#2323203)
|};

export interface Spec extends TurboModule {
Expand Down