perliedman / geojson-path-finder

Find shortest path through a network of GeoJSON
https://www.liedman.net/geojson-path-finder/
ISC License
300 stars 86 forks source link

Convert library to ES6 #54

Closed hutch120 closed 4 years ago

hutch120 commented 4 years ago

Hi Per,

This PR should fix https://github.com/perliedman/geojson-path-finder/issues/33 and https://github.com/perliedman/geojson-path-finder/issues/52

I'm almost certain the version of @turf/explode in packages.json ^5.1.0 is not compatible with the require call in topology.js without adding .default. Any version past 4.7.3 will not work without .default added to the require. Note that .require was added to the @turf/distance require in preprocessor.js, so I guess this has a precedent.

Note that I tried a few workarounds before doing this PR, like adding @turf/explode to my packages.json, but it didn't work because webpack uses the version in this packages.json (if that makes sense), not my local one.

Note that in the demo the @turf/explode module is V4.7.3, not the one in this package.

Would be awesome if you could accept this and do an NPM deploy.

Cheers, Simon

hutch120 commented 4 years ago

Hmmm, the test fails... I think it's because my create-react-app / webpack is wrapping var Queue = require('tinyqueue'); in a export default statement. So in my code I need to do .default, but in the raw library there is no babel wrapper, hence no need to do .default hence, test fails. Ahhhhggg.

hutch120 commented 4 years ago

@perliedman Hey, I've kind of gone ham on this repo. :) I've updated the whole repo to ES6 and added ESLint Standard formatting. I've taken a copy for my project, so no worries if you don't want to accept this PR. But if you do accept it , then this probably has breaking changes for <ES6 environments. BTW... cheers for the awesome library!

perliedman commented 4 years ago

Hi @hutch120,

thanks for looking into these issues. It looks like you have done quite some work here...

As you note yourself, this change is pretty intrusive, more or less touching every line of the code. It is really hard for me to accept that, especially given the risk that this breaks for <ES6 (which, for me personally, is pretty much my only use case).

While I certainly prefer the ES6 / modules style these days, given that I do not have the time to look into these changes in detail, I will have to say pass on this for now.

Having said that, I would like to keep the changes from this PR in this repo, if I have more time and these changes make more sense in the future.

Always a bit hard to turn down the result of what I guess must have been quite a lot of work on your part, but I hope you understand my reasoning. Hope to find some use for this in the future!

hutch120 commented 4 years ago

Thanks for getting back to me... Yeah, no problem, its all good, I've integrated it with my project so I have no need for the NPM package.

It wasn't that hard really, it looks like a lot more than it is due to applying ESLint Standard (which removes all the semi-colons)... probably should have done that separately to make it easier to review.

The only other change I made (but didn't check-in) was something that you implemented in the demo to get a point on the graph from a nearby waypoint. I called the function findPathWithNearbyPoints. For reference, here it is:

I added this to the constructor:

  this._points = Object.keys(this._graph.compactedVertices).map(function (coord) {
    return point(coord.split(','))
  })
  this._fc = featureCollection(this._points)

And this:

PathFinder.prototype = {
  findPathWithNearbyPoints: function (start, finish) {
    // Find closest node in base set... do this search for both at the same time?
    const startNearest = nearestPoint(start.geometry.coordinates, this._fc)
    const finishNearest = nearestPoint(finish.geometry.coordinates, this._fc)

    const startNearestCoord = startNearest.geometry.coordinates
    const finishNearestCoord = finishNearest.geometry.coordinates

    var route = this.findPath(startNearestCoord, finishNearestCoord)
    if (!route || !route.path) {
      return null
    }
    const pointArr = route.path.map(function (coords) {
      return point(coords)
    })
    route.featureCollection = featureCollection(pointArr)
    return route
  },
...