search

prettymuchbryce / easystarjs

An asynchronous A* pathfinding API written in Javascript.
MIT License
1.9k stars 165 forks source link

Return result directly from findPath with enableSync() #54

Open PSeitz opened 7 years ago

prettymuchbryce commented 7 years ago

I appreciate the change, but I'm not sure I will merge this. I think I prefer the API to be consistent in both the sync and async cases even if it's a little less convenient.

One option would be to follow the node standard modules convention in creating a findPathSync which has this behavior, but it seems unnecessary given we already have the existing configuration option, and may just be more confusion than it's worth.

PSeitz commented 7 years ago

From the API perspective sync calls usually return values, only async operations require functions callbacks, but I agree both mangled in this function is not great.

A new function may be the best way. I would remove enableSync as configuration and just provide findPathSync since enableSync and a callback is confusing.

prettymuchbryce commented 7 years ago

I think it makes sense to add the findPathSync and deprecate the enableSync option, but we can't just remove it outright otherwise we will break backwards compatibility.

mikedevelops commented 6 years ago

@prettymuchbryce Firstly, thank you for creating this great library, really impressive work.

I agree that the implementation of findPathSync would be a valuable iteration to the current enableSync method. I'm in a situation where I need to manage multiple co-dependant paths, so right now (although possible) the thought of handling them asynchronously and dealing potential race conditions (or having to manage synchronous callbacks) is putting me off.

Is there any update on this PR? It'd be great to get this in master. If there's anything I can do to help, I'm more than happy to contribute.

prettymuchbryce commented 6 years ago

@mikedevelops I understand your concerns. I think the proposal in this PR makes a lot of sense. If you are interested in contributing, I think this would be a good candidate for a first PR.

This PR is nearly completed. In my mind I see only a few remaining tasks.

  1. Restore the enableSync option that was deleted as part of this PR. We can't remove it, because we want to retain backwards compatibility so we don't break people's projects.
  2. Write some unit tests for findPathSync.
  3. Optional, but I'm noticing we don't currently have tests for enableSync, so writing a test or two for that to ensure it still works as expected after the change would be preferred.

I'm happy to answer any questions in this thread.

mikedevelops commented 6 years ago

Okay sounds great @prettymuchbryce, I'll aim to get that done by the weekend 😄

mikedevelops commented 6 years ago

@prettymuchbryce It seemed easier to create a separate PR for this, I've added findPathSync method and left all the existing functionality untouched to maintain backwards compatibility. I added those enableSync tests also 👍 .

It's open here (https://github.com/prettymuchbryce/easystarjs/pull/62) and ready for review.