Skip to content

Commit

Permalink
Fix promises on iOS to no longer wrap values in Arrays
Browse files Browse the repository at this point in the history
Summary:
public
In 9baff8f#diff-8d9841e5b53fd6c9cf3a7f431827e319R331, I incorrectly assumed that iOS was wrapping promises in an extra Array.  What was really happening is that all the callers were doing this.  I removed the wrapping in the callers and the special case handling MessageQueue.

Now one can pass whatever object one wants to resolve and it will show properly in the resolve call on the js side.  This fixes issue #5851

Reviewed By: nicklockwood

Differential Revision: D2921565

fb-gh-sync-id: 9f81e2a87f6a48e9197413b843e452db345a7ff9
shipit-source-id: 9f81e2a87f6a48e9197413b843e452db345a7ff9
  • Loading branch information
Dave Miller authored and facebook-github-bot-3 committed Feb 10, 2016
1 parent d7f7876 commit c9a1956
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 15 deletions.
10 changes: 5 additions & 5 deletions Libraries/CameraRoll/RCTCameraRollManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ @implementation RCTCameraRollManager
RCTLogWarn(@"Error saving cropped image: %@", saveError);
reject(RCTErrorUnableToSave, nil, saveError);
} else {
resolve(@[assetURL.absoluteString]);
resolve(assetURL.absoluteString);
}
}];
});
Expand All @@ -110,22 +110,22 @@ static void RCTResolvePromise(RCTPromiseResolveBlock resolve,
BOOL hasNextPage)
{
if (!assets.count) {
resolve(@[@{
resolve(@{
@"edges": assets,
@"page_info": @{
@"has_next_page": @NO,
}
}]);
});
return;
}
resolve(@[@{
resolve(@{
@"edges": assets,
@"page_info": @{
@"start_cursor": assets[0][@"node"][@"image"][@"uri"],
@"end_cursor": assets[assets.count - 1][@"node"][@"image"][@"uri"],
@"has_next_page": @(hasNextPage),
}
}]);
});
}

RCT_EXPORT_METHOD(getPhotos:(NSDictionary *)params
Expand Down
6 changes: 3 additions & 3 deletions Libraries/LinkingIOS/RCTLinkingManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ - (void)handleOpenURLNotification:(NSNotification *)notification
// TODO: we should really report success/failure via the promise here
// Doesn't really matter what thread we call this on since it exits the app
[RCTSharedApplication() openURL:URL];
resolve(@[@YES]);
resolve(@YES);
}

RCT_EXPORT_METHOD(canOpenURL:(NSURL *)URL
Expand All @@ -90,13 +90,13 @@ - (void)handleOpenURLNotification:(NSNotification *)notification
if (RCTRunningInAppExtension()) {
// Technically Today widgets can open urls, but supporting that would require
// a reference to the NSExtensionContext
resolve(@[@NO]);
resolve(@NO);
return;
}

// This can be expensive, so we deliberately don't call on main thread
BOOL canOpen = [RCTSharedApplication() canOpenURL:URL];
resolve(@[@(canOpen)]);
resolve(@(canOpen));
}

@end
6 changes: 1 addition & 5 deletions Libraries/Utilities/MessageQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,11 +324,7 @@ class MessageQueue {
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);
resolve(data);
},
(errorData) => {
var error = createErrorFromErrorData(errorData);
Expand Down
2 changes: 1 addition & 1 deletion React/Modules/RCTClipboard.m
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ - (dispatch_queue_t)methodQueue
rejecter:(__unused RCTPromiseRejectBlock)reject)
{
UIPasteboard *clipboard = [UIPasteboard generalPasteboard];
resolve(@[RCTNullIfNil(clipboard.string)]);
resolve(RCTNullIfNil(clipboard.string));
}

@end
2 changes: 1 addition & 1 deletion React/Modules/RCTSourceCode.m
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ - (void)setScriptText:(NSString *)scriptText {}
if (RCT_DEV && self.scriptData && self.scriptURL) {
NSString *scriptText = [[NSString alloc] initWithData:self.scriptData encoding:NSUTF8StringEncoding];

resolve(@[@{@"text": scriptText, @"url": self.scriptURL.absoluteString}]);
resolve(@{@"text": scriptText, @"url": self.scriptURL.absoluteString});
} else {
reject(RCTErrorUnavailable, nil, RCTErrorWithMessage(@"Source code is not available"));
}
Expand Down

8 comments on commit c9a1956

@mkonicek
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @dmmiller!

@corbt
Copy link
Contributor

@corbt corbt commented on c9a1956 Feb 11, 2016

Choose a reason for hiding this comment

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

Cool, I was bashing my head in just yesterday trying to figure out why all the RCTPromiseResolveBlocks have to return arrays!

@dmmiller
Copy link

Choose a reason for hiding this comment

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

@corbt Glad your head can heal now. Sorry about the bug. A few others had found issues too.

@kanerogers
Copy link

Choose a reason for hiding this comment

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

I love you @dmmiller. ❤️

@dmmiller
Copy link

Choose a reason for hiding this comment

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

lol, thanks :)

@kanerogers
Copy link

Choose a reason for hiding this comment

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

@dmmiller Thinking long term about this, is there any kind of integration test that could potentially pick up on these kinds of bugs in future? It kind of freaks me out that the Native --> JS interop could just blow up like that. 😞

@dmmiller
Copy link

Choose a reason for hiding this comment

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

@kanerogers we do have tests around this. The problem in this case was that we had so few places using promises and on iOS they had made one assumption and I hadn't realized it until I dug further. I think we actually have a lot of test infrastructure in place that tests the Native -> JS interop. This was just a new piece and I made a mistake when getting it working initially.

@kanerogers
Copy link

@kanerogers kanerogers commented on c9a1956 Mar 8, 2016 via email

Choose a reason for hiding this comment

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

Please sign in to comment.