API consistency in React Native

[satya164]

Currently React Native has lots of incosnistencies in its APIs. This is an attempt to settle on a standard way for API methods and change the current APIs to the standard format by discussing a migration path.

Currently available modules and their callbacks

The available APIs can be categorized into the following categories,

1. Node-style error-first callbacks

// https://facebook.github.io/react-native/docs/asyncstorage.html#content
AsyncStorage.getItem(key: string, callback?: ?(error: ?Error, result: ?string) => void) // all AsyncStorage methods

2. Value first callbacks

// https://facebook.github.io/react-native/docs/netinfo.html#content
NetInfo.isConnectionExpensive(callback: (metered: ?boolean, error?: string) => void)

3. Separate callbacks for success and error, success-first

// https://facebook.github.io/react-native/docs/cameraroll.html#content
CameralRoll.saveImageWithTag(tag, successCallback, errorCallback);
CameralRoll.getPhotos(params, callback, errorCallback);

// https://facebook.github.io/react-native/docs/nativemethodsmixin.html#content
NativeMethodsMixin.measureLayout(relativeToNativeNode: number, onSuccess: MeasureLayoutOnSuccessCallback, onFail: () => void)

4. Separate callbacks for success and error, error-first

// https://facebook.github.io/react-native/docs/actionsheetios.html#content
ActionSheetIOS.showShareActionSheetWithOptions(options: Object, failureCallback: Function, successCallback: Function)

5. Callbacks with no error

// https://facebook.github.io/react-native/docs/actionsheetios.html#content
ActionSheetIOS.showActionSheetWithOptions(options: Object, callback: Function)

// https://facebook.github.io/react-native/docs/alertios.html#prompt
Alert.prompt(title: string, value?: string, buttons?: Array<{ text?: string; onPress?: ?Function; style?: AlertButtonStyle; }>, callback?: Function)

// Clipboard: not documented yet
Clipboard.getString(callback: Function)

// https://facebook.github.io/react-native/docs/intentandroid.html#content
IntentAndroid.canOpenURL(url: string, callback: Function)
IntentAndroid.getInitialURL(callback: Function)

// https://facebook.github.io/react-native/docs/linkingios.html#content
LinkingIOS.canOpenURL(url: string, callback: Function)

// https://facebook.github.io/react-native/docs/nativemethodsmixin.html#content
NativeMethodsMixin.measure(callback: MeasureOnSuccessCallback)

// https://facebook.github.io/react-native/docs/pushnotificationios.html#content
PushNotificationIOS.getApplicationIconBadgeNumber(callback: Function)
PushNotificationIOS.checkPermissions(callback: Function)

6. Promise based APIs

// https://facebook.github.io/react-native/docs/asyncstorage.html#content
AsyncStorage // all AsyncStorage methods

// https://facebook.github.io/react-native/docs/netinfo.html#content
NetInfo.fetch().done(callback: Function)

Candidates for the standardized callbacks

The callbacks should encourage error handling IMO. Keeping that in mind, I think the following should be good candidates.

1. Node-style error-first callbacks (enforces error handling)
2. Separate callbacks for success and error, success-first (only by enforcing it in the module manually)
3. Separate callbacks for success and error, error-first (enforces error handling)
4. Promise based APIs (when error logging is implemented and errors show redbox - Add basic rejection tracking then/promise#116)

Problems to discuss

1. Discuss on the advantages and disadvantages for all of the above types of callbacks and settle on one
2. Think of a good migration path, for example when someone is using the incorrect API, show a redbox saying that API is different

If I missed any, please edit the post and add them. Thanks :D

cc @vjeux @mkonicek @nicklockwood @ide @brentvatne @skevy

[fabianeichinger]

I think consistent use of ES6 Promises - optionally with the additional non-standard done() method (NetInfo.fetch() returns a Promise, so NetInfo.fetch().done(callback: Function) probably shouldn't be part of category 5) - is the way to go. It gives us async-await syntax in ES2016 and probably the best compatibility with libraries which consume async data.

Also if the decision for Promises everywhere is made, use-sites of other callback styles should probably be deprecated, except for uses that are clearly not a Promise, e.g. event listening with addEventListener.

[ippa]

What feichngr said - I would go with promises. Love working with fetch() in RN right now. Functions returning promises makes for some nice composability.

[satya164]

Yeah, personally I think that promises are the way to go. Currently the problem with promises in React native is that errors are not shown unless you call .done(), which is easy to forget, and also non-standard. But it seems that this is going to be fixed soon - then/promise#116 and then it'll be a good candidate for the APIs.

[corbt]

Another vote for promises, especially since babel lets us use async/await in RN. I also agree though that unhandled errors in promises should be presented as a redbox, otherwise it's too easy to not realize that something has failed.

[brentvatne]

Nice writeup @satya164 :) Another vote for promises

[satya164]

@davidaurelio is the promise polyfill with error tracking being used in RN yet?

[davidaurelio]

No, will update it… thanks for the reminder!

[skevy]

@davidaurelio the Promise polyfill is going to live in fbjs after this PR is merged, fyi: #5084

[davidaurelio]

@skevy thanks for the heads up.

[satya164]

Now that RN has rejection tracking for promises, we could get started on this. @skevy

[corbt]

This may be opening a whole different can of worms, but I think it would be worth considering including/standardizing on an RxJS Observable pattern for stuff that emits event streams like NetInfo.addEventListener. It could be used like:

// Basic example to print all connectivity events
NetInfo.connectivityEvents.subscribe(connection => console.log(`current connection: ${connection}`));

// Start sync when connected on wifi
NetInfo.connectivityEvents.filter(connection => connection === 'wifi').subscribe(startSync);

[ide]

This may be opening a whole different can of worms

I think so. Also word through the grapevine is some bigger cos are moving away from Observable.

[grabbou]

Is this still something we should keep opened? Looks like we have decided to go with Promise-based API, that's what other libraries started to adapting and the general API consistency issue mentioned seems to be solved (if there are any components / APIs in a need of update, I believe we can handle them in smaller issues).

[satya164]

@grabbou Yeah, we can file smaller issues and close this.

[mkonicek]

Yeah, we can file smaller issues and close this.

OK I'll close this one. Just saw the description again, it's an awesome writeup! Have a lot of APIs been migrated to Promises?

[satya164]

@mkonicek Thank you :)

Yeah, most have been migrated to promises. I think only PushNotificationIOS and NativeMethodsMixin are remaining from this post.

[grabbou]

And.... ActionSheetIOS but there's open PR for that :)