Deprecate AlertMacOS in favor of Alert #662

thymikee · 2020-12-02T23:16:17Z

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Summary

Fixes #354 by introducing an internal promptMacOS function to Alert module, copying implementation from AlertMacOS. Figured that would be the least disruptive and non-breaking way of introducing this, as it allows us to put the deprecation warning directly inside AlertMacOS.prompt, also called by AlertMacOS.alert.

Builds on top of #661, to be rebased once it's merged.

Changelog

[macOS] [Deprecated] - Deprecate AlertMacOS in favor of Alert

Test Plan

Existing RNTester tests.

Microsoft Reviewers: Open in CodeFlow

thymikee · 2020-12-02T23:20:33Z

Libraries/Alert/Alert.js

@@ -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);


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?

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

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

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.

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 :)

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.

harrieshin · 2020-12-08T02:33:02Z

React/CoreModules/RCTAlertController.h

+#define UIAlertController NSViewController
+#endif // ]TODO(macOS ISS#2323203)
+
+@interface RCTAlertController : UIAlertController


@HeyImChris @thymikee : we are adding subclass of uialertcontroller when Apple documentation says "The UIAlertController class is intended to be used as-is and does not support subclassing. The view hierarchy for this class is private and must not be modified." ?

https://developer.apple.com/documentation/uikit/uialertcontroller?language=objc

Right, didn't notice that. It comes from the upstream facebook@f319ff3

HeyImChris · 2020-12-11T21:55:12Z

Libraries/Alert/NativeAlertManager.js

+  // [TODO(macOS ISS#2323203)
+  defaultInputs?: DefaultInputsArray,
+  modal?: ?boolean,
+  critical?: ?boolean,


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

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

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

Saadnajmi · 2022-12-08T23:10:41Z

Closing in favor of #1548 . Thanks @thymikee, I'll try and make sure your original changes show up as a single merged commit when it lands :)

thymikee · 2022-12-09T09:51:43Z

Thanks @Saadnajmi, appreciated! Glad it finally made through :D

thymikee requested a review from alloy as a code owner December 2, 2020 23:16

thymikee commented Dec 2, 2020

View reviewed changes

harrieshin reviewed Dec 8, 2020

View reviewed changes

HeyImChris suggested changes Dec 11, 2020

View reviewed changes

ghost added the Needs: Author Feedback label Dec 11, 2020

thymikee added 5 commits December 14, 2020 11:27

address feedback; fix iOS build

a5723c1

Deprecate AlertMacOS in favor of Alert

27ebe50

move UIAlertController define to RCTAlertController.h

3d87b36

define only on macos target

1cb846e

revert podfile.lock

8973d60

thymikee force-pushed the macos/deprecate-alertmacos branch from 649076c to 8973d60 Compare December 14, 2020 10:30

ghost removed the Needs: Author Feedback label Dec 14, 2020

Saadnajmi self-assigned this Dec 6, 2022

Saadnajmi mentioned this pull request Dec 6, 2022

Remove AlertMacOS in favor of Alert & promptMacOS #1548

Merged

4 tasks

Saadnajmi closed this Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate AlertMacOS in favor of Alert #662

Deprecate AlertMacOS in favor of Alert #662

thymikee commented Dec 2, 2020 •

edited by ghost

Loading

thymikee Dec 2, 2020

alloy Dec 3, 2020

thymikee Dec 3, 2020

NickGerleman Dec 3, 2020

thymikee Dec 4, 2020

HeyImChris Dec 4, 2020

harrieshin Dec 8, 2020

harrieshin Dec 8, 2020

thymikee Dec 8, 2020

HeyImChris Dec 11, 2020

alloy Dec 14, 2020

thymikee Dec 14, 2020

Saadnajmi commented Dec 8, 2022

thymikee commented Dec 9, 2022

Deprecate AlertMacOS in favor of Alert #662

Deprecate AlertMacOS in favor of Alert #662

Conversation

thymikee commented Dec 2, 2020 • edited by ghost Loading

Please select one of the following

Summary

Changelog

Test Plan

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saadnajmi commented Dec 8, 2022

thymikee commented Dec 9, 2022

thymikee commented Dec 2, 2020 •

edited by ghost

Loading