Skip to content

Commit

Permalink
Make CameraRoll work with Promises
Browse files Browse the repository at this point in the history 
Summary:
public
This is the first module moving to the new model of working with Promises.

We now warn on uses of callback version.  At some point we will remove that.

Reviewed By: davidaurelio

Differential Revision: D2849811

fb-gh-sync-id: 8a31924cc2b438efc58f3ad22d5f27c273563472
  • Loading branch information
Dave Miller authored and facebook-github-bot-2 committed Jan 21, 2016
1 parent 34d5fa2 commit 9baff8f
Show file tree
Hide file tree
Showing 7 changed files with 105 additions and 95 deletions.
69 changes: 34 additions & 35 deletions Libraries/CameraRoll/CameraRoll.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,57 +128,56 @@ class CameraRoll {
* - a tag not matching any of the above, which means the image data will
* be stored in memory (and consume memory as long as the process is alive)
*
* @param successCallback Invoked with the value of `tag` on success.
* @param errorCallback Invoked with error message on error.
* Returns a Promise which when resolved will be passed the new uri
*
*/
static saveImageWithTag(tag, successCallback, errorCallback) {
static saveImageWithTag(tag) {
invariant(
typeof tag === 'string',
'CameraRoll.saveImageWithTag tag must be a valid string.'
);
RCTCameraRollManager.saveImageWithTag(
tag,
(imageTag) => {
successCallback && successCallback(imageTag);
},
(errorMessage) => {
errorCallback && errorCallback(errorMessage);
});
if (arguments.length > 1) {
console.warn("CameraRoll.saveImageWithTag(tag, success, error) is deprecated. Use the returned Promise instead");
let successCallback = arguments[1];
let errorCallback = arguments[2] || ( () => {} );
RCTCameraRollManager.saveImageWithTag(tag).then(successCallback, errorCallback);
return;
}
return RCTCameraRollManager.saveImageWithTag(tag);
}

/**
* Invokes `callback` with photo identifier objects from the local camera
* Returns a Promise with photo identifier objects from the local camera
* roll of the device matching shape defined by `getPhotosReturnChecker`.
*
* @param {object} params See `getPhotosParamChecker`.
* @param {function} callback Invoked with arg of shape defined by
* `getPhotosReturnChecker` on success.
* @param {function} errorCallback Invoked with error message on error.
*
* Returns a Promise which when resolved will be of shape `getPhotosReturnChecker`
*
*/
static getPhotos(params, callback, errorCallback) {
var metaCallback = callback;
static getPhotos(params) {
if (__DEV__) {
getPhotosParamChecker({params}, 'params', 'CameraRoll.getPhotos');
invariant(
typeof callback === 'function',
'CameraRoll.getPhotos callback must be a valid function.'
);
invariant(
typeof errorCallback === 'function',
'CameraRoll.getPhotos errorCallback must be a valid function.'
);
}
if (__DEV__) {
metaCallback = (response) => {
getPhotosReturnChecker(
{response},
'response',
'CameraRoll.getPhotos callback'
);
callback(response);
};
if (arguments.length > 1) {
console.warn("CameraRoll.getPhotos(tag, success, error) is deprecated. Use the returned Promise instead");
let successCallback = arguments[1];
if (__DEV__) {
let callback = arguments[1];
successCallback = (response) => {
getPhotosReturnChecker(
{response},
'response',
'CameraRoll.getPhotos callback'
);
callback(response);
};
}
let errorCallback = arguments[2] || ( () => {} );
RCTCameraRollManager.getPhotos(params).then(successCallback, errorCallback);
}
RCTCameraRollManager.getPhotos(params, metaCallback, errorCallback);
// TODO: Add the __DEV__ check back in to verify the Promise result
return RCTCameraRollManager.getPhotos(params);
}
}

Expand Down
45 changes: 24 additions & 21 deletions Libraries/CameraRoll/RCTCameraRollManager.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,43 +79,46 @@ @implementation RCTCameraRollManager

@synthesize bridge = _bridge;

NSString *const RCTErrorUnableToLoad = @"E_UNABLE_TO_LOAD";
NSString *const RCTErrorUnableToSave = @"E_UNABLE_TO_SAVE";

RCT_EXPORT_METHOD(saveImageWithTag:(NSString *)imageTag
successCallback:(RCTResponseSenderBlock)successCallback
errorCallback:(RCTResponseErrorBlock)errorCallback)
resolve:(RCTPromiseResolveBlock)resolve
reject:(RCTPromiseRejectBlock)reject)
{
[_bridge.imageLoader loadImageWithTag:imageTag callback:^(NSError *loadError, UIImage *loadedImage) {
if (loadError) {
errorCallback(loadError);
reject(RCTErrorUnableToLoad, nil, loadError);
return;
}
// It's unclear if writeImageToSavedPhotosAlbum is thread-safe
dispatch_async(dispatch_get_main_queue(), ^{
[_bridge.assetsLibrary writeImageToSavedPhotosAlbum:loadedImage.CGImage metadata:nil completionBlock:^(NSURL *assetURL, NSError *saveError) {
if (saveError) {
RCTLogWarn(@"Error saving cropped image: %@", saveError);
errorCallback(saveError);
reject(RCTErrorUnableToSave, nil, saveError);
} else {
successCallback(@[assetURL.absoluteString]);
resolve(@[assetURL.absoluteString]);
}
}];
});
}];
}

static void RCTCallCallback(RCTResponseSenderBlock callback,
NSArray<NSDictionary<NSString *, id> *> *assets,
BOOL hasNextPage)
static void RCTResolvePromise(RCTPromiseResolveBlock resolve,
NSArray<NSDictionary<NSString *, id> *> *assets,
BOOL hasNextPage)
{
if (!assets.count) {
callback(@[@{
resolve(@[@{
@"edges": assets,
@"page_info": @{
@"has_next_page": @NO,
}
}]);
return;
}
callback(@[@{
resolve(@[@{
@"edges": assets,
@"page_info": @{
@"start_cursor": assets[0][@"node"][@"image"][@"uri"],
Expand All @@ -126,8 +129,8 @@ static void RCTCallCallback(RCTResponseSenderBlock callback,
}

RCT_EXPORT_METHOD(getPhotos:(NSDictionary *)params
successCallback:(RCTResponseSenderBlock)successCallback
errorCallback:(RCTResponseErrorBlock)errorCallback)
resolve:(RCTPromiseResolveBlock)resolve
reject:(RCTPromiseRejectBlock)reject)
{
NSUInteger first = [RCTConvert NSInteger:params[@"first"]];
NSString *afterCursor = [RCTConvert NSString:params[@"after"]];
Expand All @@ -137,7 +140,7 @@ static void RCTCallCallback(RCTResponseSenderBlock callback,

BOOL __block foundAfter = NO;
BOOL __block hasNextPage = NO;
BOOL __block calledCallback = NO;
BOOL __block resolvedPromise = NO;
NSMutableArray<NSDictionary<NSString *, id> *> *assets = [NSMutableArray new];

[_bridge.assetsLibrary enumerateGroupsWithTypes:groupTypes usingBlock:^(ALAssetsGroup *group, BOOL *stopGroups) {
Expand All @@ -157,9 +160,9 @@ static void RCTCallCallback(RCTResponseSenderBlock callback,
*stopAssets = YES;
*stopGroups = YES;
hasNextPage = YES;
RCTAssert(calledCallback == NO, @"Called the callback before we finished processing the results.");
RCTCallCallback(successCallback, assets, hasNextPage);
calledCallback = YES;
RCTAssert(resolvedPromise == NO, @"Resolved the promise before we finished processing the results.");
RCTResolvePromise(resolve, assets, hasNextPage);
resolvedPromise = YES;
return;
}
CGSize dimensions = [result defaultRepresentation].dimensions;
Expand Down Expand Up @@ -188,18 +191,18 @@ static void RCTCallCallback(RCTResponseSenderBlock callback,
}
}];
} else {
// Sometimes the enumeration continues even if we set stop above, so we guard against calling the callback
// Sometimes the enumeration continues even if we set stop above, so we guard against resolving the promise
// multiple times here.
if (!calledCallback) {
RCTCallCallback(successCallback, assets, hasNextPage);
calledCallback = YES;
if (!resolvedPromise) {
RCTResolvePromise(resolve, assets, hasNextPage);
resolvedPromise = YES;
}
}
} failureBlock:^(NSError *error) {
if (error.code != ALAssetsLibraryAccessUserDeniedError) {
RCTLogError(@"Failure while iterating through asset groups %@", error);
}
errorCallback(error);
reject(RCTErrorUnableToLoad, nil, error);
}];
}

Expand Down
20 changes: 16 additions & 4 deletions Libraries/Utilities/MessageQueue.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
let Systrace = require('Systrace');
let ErrorUtils = require('ErrorUtils');
let JSTimersExecution = require('JSTimersExecution');
let Platform = require('Platform');

let invariant = require('invariant');
let keyMirror = require('keyMirror');
Expand Down Expand Up @@ -318,10 +319,21 @@ class MessageQueue {
if (type === MethodTypes.remoteAsync) {
fn = function(...args) {
return new Promise((resolve, reject) => {
self.__nativeCall(module, method, args, resolve, (errorData) => {
var error = createErrorFromErrorData(errorData);
reject(error);
});
self.__nativeCall(
module,
method,
args,
(data) => {
// iOS always wraps the data in an Array regardless of what the
// shape of the data so we strip it out
// Android sends the data back properly
// TODO: Remove this once iOS has support for Promises natively (t9774697)
resolve(Platform.OS == 'ios' ? data[0] : data);

This comment has been minimized.

Sign in to view

Copy link
@dmmiller

dmmiller Jan 21, 2016

@ide Notice this change to iOS promises. The parameter is no longer wrapped in an array. It is just whatever object is passed from the native side. I think you might be the only person relying on this. I changed it to match Android which doesn't put the extra wrapper around it. Hopefully I didn't break you or not too badly anyway.

This comment has been minimized.

Sign in to view

Copy link
@geof90

geof90 Feb 9, 2016

Contributor

@dmmiller Why is this needed? I'm facing issues with my native module in RN 0.20-rc1.

screen shot 2016-02-09 at 3 50 06 pm

Reverting this change made everything work fine again.
},
(errorData) => {
var error = createErrorFromErrorData(errorData);
reject(error);
});
});
};
} else {
Expand Down
4 changes: 1 addition & 3 deletions Libraries/Utilities/Platform.android.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@

'use strict';

var AndroidConstants = require('NativeModules').AndroidConstants;

var Platform = {
OS: 'android',
Version: AndroidConstants.Version,
get Version() { return require('NativeModules').AndroidConstants.Version; },
};

module.exports = Platform;
4 changes: 3 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/bridge/Promise.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

package com.facebook.react.bridge;

import javax.annotation.Nullable;

/**
* Interface that represents a JavaScript Promise which can be passed to the native module as a
* method parameter.
Expand All @@ -22,5 +24,5 @@ public interface Promise {
@Deprecated
void reject(String reason);
void reject(String code, Throwable extra);
void reject(String code, String reason, Throwable extra);
void reject(String code, String reason, @Nullable Throwable extra);
}
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/bridge/PromiseImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public void reject(String code, Throwable extra) {
}

@Override
public void reject(String code, String reason, Throwable extra) {
public void reject(String code, String reason, @Nullable Throwable extra) {
if (mReject != null) {
if (code == null) {
code = DEFAULT_ERROR;
Expand Down
Loading

4 comments on commit 9baff8f

@dmmiller
Copy link

Choose a reason for hiding this comment

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

@satya164 Here is the change to CameraRoll we were talking about.

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

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

@dmmiller Thanks.

@samuelkraft
Copy link

Choose a reason for hiding this comment

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

Is there anywhere to see an example use with promises? The docs seems outdated.

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

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

@samuelkraft It's the same API. Just that it returns a promise rather than taking success and error callbacks. A PR will be awesome to update the docs.

Please sign in to comment.