Proposal: replace callbacks by Promises

[Chnapy]

Hi,

I was wondering if there is any reason Easystar should not use Promise objects. My idea is that instead of using callbacks given to findPath(), the function calculate() would simply return an array of Promise (one for each path).

The only concern I see is the ES6 requirement, but Babel is here for that.

Promises are used today in a lot of libs and apps, they are normalized and quite easy to use. In my app I wrap the callback in a Promise, what is not very elegant.

I can do a PR, I just want some returns before digging the code. What do you think ? :shipit:

[prettymuchbryce]

Hey Chnapy. I think it's a good idea. This library was written before Promises were standardized in JavaScript.

Introducing native promises would be a breaking change. So I think it should be introduced as part of 1.x.x.

I am open to hearing proposals about how the API would be affected by this change.

One thing that comes to mind. Currently findPath returns an instanceId which can be used to cancel the path calculation. If findPath instead returns a promise, how do we handle canceling the path calculation?

[Chnapy]

Hi !

Thanks for your answer, I see the 'cancel' problematic. I see 2 solutions:

The first one In my side, my wrapper returns a list of PathObject which is composed by the promise, and a cancel() function:

export interface PathObject {
    promise: Promise<Position[]>; // Position = {x,y}
    cancel: () => boolean;
}

So calculate() would return a list of this PathObject in which cancel() would cancel the calculation. The change would not be so hard, but the initial logic changes maybe not for the better.

The second one As I see, Easystar store its instances in an object instances following this type:

var instances: {
  [id: number]: Instance;
};

My suggestion is to instead use a Map. Maps can have objects as keys, like Promises. In maps the object key check is done by reference, not by value.

var instances: Map<Promise, Instance>;

So now we can do this:

finder.findPath(startX, startY, endX, endY);

const promises = finder.calculate();

// On first path done, do something
promises[0].then(path => console.log(path));

// I want to cancel the first path
finder.cancelPath(promises[0])

I cancel the path calculation by simply giving the promise itself.

I like this solution, it keeps the initial logic, and I find it quite elegant. But the type change of instances may be too much ?

[pkaminski]

A better option might be to dynamically add a cancel() method to the promise being returned, which could capture the id and invoke cancelPath when called.