stellar / js-stellar-sdk

Main Stellar client library for the JavaScript language.
https://stellar.github.io/js-stellar-sdk/
Apache License 2.0
621 stars 305 forks source link

use async/await #296

Open Akuukis opened 5 years ago

Akuukis commented 5 years ago

Let's rewrite Promises to asyncs. Here's why:

function test(): Promise<string> {
    if(Math.random() < 0.5) throw Error('Unlucky')

    return Promise.resolve('lucky')
}

function promiseStyle() {
    test()
        .then((res) => console.log(res))
        .catch((err) => console.error(err))  // <-- this will not execute.
}

async function asyncStyle() {
    try {
        console.log(await test())
    } catch(err) {
        console.error(err)  // <-- this will execute.
    }
}

That's fair not to assume what consumers of stellar-sdk will prefer: promises or async. But there's one really tricky thing on how the function test is written: as you see, promise people will be surprised. If it were async function test instead, then both would execute as expected.

In my opinion call this a very subtle trap that's not nice for sdk consumers, and therefore I propose to use async over promises by default. Because using Promises the trap show in function test may happen, while using async's it's impossible by design.

In fact, you can find such example at CallBuilder.call method. Unfortunely, tests are testing for such trap to happen there. So it will require a "slightly breaking change" to solve this fully.

vcarl commented 4 years ago

That's due to how the promise is being made, returning Promise.resolve means only the success case is promise-ified. Replacing your test with

function test() {
  return new Promise(() => {
    if (Math.random() < 0.5) throw Error("Unlucky");

    return "lucky";
  });
}

works as you desire.

Personally I think this isn't an issue and not worth rewriting to a new style.