google / promises

Promises is a modern framework that provides a synchronization construct for Swift and Objective-C.
Apache License 2.0
3.8k stars 295 forks source link

FBLPromise+Wrap.m fie some syntax error #164

Closed iMinder closed 3 years ago

iMinder commented 3 years ago

in + (FBLPromise<NSArray > )onQueue:(dispatch_queue_t)queue wrap2ObjectsOrErrorCompletion:(void (^)(FBLPromise2ObjectsOrErrorCompletion))work function, which means that value1 and value2 can be nullable. But when I pass value1 or value 2 nil, the app will crash. before fulfill(@[ value1, value2 ]); I think you should determin whether value1 or value2 is nil.

shoumikhin commented 3 years ago

Hi iMinder,

Looks like you're trying to create an NSArray with one of the values being nil? Containers don't support nil values, but you can use NSNull to store it instead of nil.

Thanks.

iMinder commented 3 years ago

But I wrap results with FBLPromise2ObjectsOrErrorCompletion block, which supports nil parameters. Here is my test code.

- (FBLPromise<NSArray *> *)fetchWithURL:(NSURL *)url {
  return [FBLPromise wrap2ObjectsOrErrorCompletion:^(FBLPromise2ObjectsOrErrorCompletion handler) {
      dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(3 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
          handler(nil, @"123", nil);
      });
  }];
}
shoumikhin commented 3 years ago

Oh, that's actually our bug, great you caught it!

To fix it, we likely need to change this line to:

fulfill(@[ value1 ?: [NSNull null], value2 ?: [NSNull null] ]);

Can you please verify it works on your end, and also change the test to pass nil for one of the values (and expect NSNull in the resulting array), and send a pull-request our way?

Thanks!

iMinder commented 3 years ago

Oh, that's actually our bug, great you caught it!

To fix it, we likely need to change this line to:

fulfill(@[ value1 ?: [NSNull null], value2 ?: [NSNull null] ]);

Can you please verify it works on your end, and also change the test to pass nil for one of the values (and expect NSNull in the resulting array), and send a pull-request our way?

Thanks!

I test this code and it works.

shoumikhin commented 3 years ago

@iMinder thanks for confirming! Would you mind doing the aforementioned change and submitting it as a pull-request then?

sushichop commented 3 years ago

This issue should be fixed by #166 🙂

ykjchen commented 3 years ago

Available in 1.2.12.