Add findPathSync to find & calculate a path then return the result directly

[mikedevelops]

The proposal is an implementation similar to (#54).

This change would not remove or modify from the existing public API or existing enableSync() method. It adds an additional method to the public API that allows users to find a path and calculate it's route truly synchronously with no callback.

Example usage...

var easyStar = new EasyStar.js();
var grid = [
    [0, 1, 0],
    [0, 0, 0],
    [0, 0, 0],
];
easyStar.setGrid(grid);
easyStar.setAcceptableTiles([0]);

const path = easyStar.findPathSync(0, 0, 1, 1); // your path array or null

I've done some semicolon and JSDoc housekeeping along the way, as well as added tests for the existing enableSync() behaviour.

[mikedevelops]

This is likely the most controversial change ☝️ , I'm happy to discuss this, besides from deferring the callback onto the next execution stack, I'm not sure what is gained from wrapping this in a setTimeout for the asynchronous mode.

[prettymuchbryce]

This is important as without it we would be introducing zalgo bugs. The fact that the callback is asynchronous should be consistent.

See here: http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony

[mikedevelops]

That is a really interesting read, not something that I've ever come across. I'm not certain I 100% understand it in our context, but I'm more than happy to revert this.

[prettymuchbryce]

In our context it is relevant because

If you have an API which takes a callback,
and sometimes that callback is called immediately,
and other times that callback is called at some point in the future,
then you will render any code using this API impossible to reason about, and cause the release of Zalgo.

If sync is not enabled, we want to consistently call the callback asynchronously. Removing the setTimeout means the callback could be called immediately, making the callback behavior inconsistent.

[prettymuchbryce]

When you revert the Zalgo bug this test won't work anymore. I would recommend instead to set the iterationsPerCalculation value to be value low (like 1), a single calculate() call should still find the path.

[mikedevelops]

Interestingly this test does still work with the zalgo bug reverted. So I understand, when would you expect array referenced by result to be updated in this context?

This is the enableSync test, so our callback is immediately invoked on the (I assume) current execution stack. Therefore our result will have changed before calculate has returned as calculate is synchronous. We then run the assertion which will take place after calculate has returned.

[prettymuchbryce]

My expectation would be that if you reverted the change to the setTimeout, then result would be an empty array until we enter the callback, because the callback is executed only after the assertions below calculate().

[prettymuchbryce]

Thanks very much for the change. This is looking great! Just a couple of small comments around the Zalgo stuff. Thanks for the additional cleanup as well!

Could you bump the minor version in package.json and change it throughout the code? It's kind of annoying right now, as you need to change it in a number of places, so alternatively you could simply remove the files changed in bin and I can create the built files when I go to bump the version and release.