js-stellar-sdk icon indicating copy to clipboard operation
js-stellar-sdk copied to clipboard

use async/await

Open Akuukis opened this issue 6 years ago • 1 comments

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.

Akuukis avatar May 24 '19 12:05 Akuukis

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.

vcarl avatar Nov 11 '19 22:11 vcarl