Broken Promises Flow

[rzpamidi]

Different behaviour has been noted with the promise returned, when a callback is passed to the api function.

eg.

const instance = new Razorpay({
  key_id: 'rzp_test_2mYdwI91aUdL2u',
  key_secret: 'hd0FgTKnZUUJCGYHTdfQ6Zx2'
});

instance.payments.all({})
  .then((arg) => console.log(arg)) // arg value is printed  
  .catch(arg => console.log(arg))

instance.payments.all({}, function () {
  console.log(arguments); // arguments are printed
}).then((arg) => console.log(arg)) // arg value is undefined 
  .catch(arg => console.log(arg))

[selvagsz]

IMO, a promise shouldn't be returned when a callback is passed

[7ranveer]

The callback function does not return a promise so it will never be resolved in .then() function. I think below code might work instance.payments.all({}, function () { console.log(arguments); // arguments are printed return new Promise( (resolve,reject)=>{ if(arguments){ resolve(arguments) }else{ reject("no arguments") } ) }).then((arg) => console.log(arg)) .catch(arg => console.log(arg))

[rzpamidi]

We shall give the same arguments that we passed to callback , to the then callback also, to make this behaviour consistent

[selvagsz]

Before promises, error-first callbacks are the only paradigm to propagate errors thru the async chain. Promises have a declarative approach of error-handling now. It may look weird to resolve a promise with errors as first param

[rzpamidi]

@selvagsz ,

It may look weird to resolve a promise with errors as first param

We will not do that

we support both error-first call back and promises right ? , let the behaviour be consistent that 1) If you pass a callback , it would get err , data as arguments 2) If you use Promise Chain , in case of error the promise is rejected else the promise is resolved.

The above points mentioned work well independently. Let 'em work well together also. Resolving with different arguments when callback is passed is not good.

[selvagsz]

[Question] Why would people do both ? Should we encourage that pattern of using both ?

imo, don't think that's good pattern; callbacks exists for legacy node code. Maybe we can check some other node api wrappers

[rzpamidi]

@selvagsz , node throws an error if we do not provide callback (eg. fs.readFile), which we can not enforce, also all our api's support both, i see no way than supporting both.

We can use the fix i mentioned for backwards compatibility , we can decide later to not return promise when callback is passed, and do Major version bump.

[rzpamidi]

Should we encourage that pattern of using both ?

I don't see any harm , it covers all kinds of users without much overhead , where as the current implementation is inconsistent.

[captn3m0]

@rzpamidi Can we close this?

[rzpamidi]

@captn3m0 , need to implement the changes required. will close it after that