perliedman / geojson-path-finder

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

distance is not a function #39

Closed zeromonad closed 6 years ago

zeromonad commented 6 years ago

Can you help with resolving this? Not sure what the issue is.

TypeError: distance is not a function9 | module.exports = function preprocess(graph, options) { 10 | options = options || {}; 11 | var weightFn = options.weightFn || function defaultWeightFn(a, b) { 12 | return distance(point(a), point(b)); 13 | }, 14 | topo; 15 |

zeromonad commented 6 years ago

Never mind, I think it's an issue on my end due to mixing ES5 and ES6.

qunabu commented 6 years ago

How did you managed to fix this ?

Im trying to implement this in create-react-app experiencing same error

zeromonad commented 6 years ago

Reason it's happening is because it's a node module and requires node.js (create-react-app is React only). I just fixed it up by spinning a up a local node.js server and routing my requests through that.

qunabu commented 6 years ago

Actually it does works with create-react-app in the browser. The issue is with require function which behaves differently in webpack.

Changing one line in src/geojson-path-finder/preprocessor.js from

 distance = require('@turf/distance'),

to

 distance = require('@turf/distance').default,

does make is work with webpack, and paths can be found now on frontend as well.

Im assuming that this could be fixed in webpack.config

zeromonad commented 6 years ago

Qunabu, I love you. Thank you so much.

prusswan commented 6 years ago

Agreed..was stumped by this for quite awhile

One way to fix this from webpack side is to specify resolve.mainFields as ['main', 'module'], e.g.:

environment.config.set('resolve.mainFields', ['main', 'module'])

Still looking for better ways that do not affect the loading of other modules.. (this might work for webpack 4: https://github.com/webpack/webpack/issues/6796)

More info: https://github.com/webpack/webpack/issues/4742#issuecomment-300041285

davidgilbertson commented 5 years ago

I don't think this should be closed. It means that we can't use geojson-path-finder without ejecting a create-react-app and/or creating an exception for this package, is that right?

@perliedman I'm keen to use this package but can't justify ejecting my app. Are you still looking at issues here? Would you accept a PR that fixes the issue?

If not please let me know and I'll create a fork. Thanks!

prusswan commented 5 years ago

I think this is more a turf or webpack/commonjs issue, but if there is a more compatible way of loading turf/distance by all means suggest it

davidgilbertson commented 5 years ago

You're right, the problem isn't with geojson-path-finder, or Turf, it's with Webpack. But as far as I can work out, it can only be fixed with changes to each user's Webpack config, or a change to how geojson-path-finder imports.

The problem: @turf/distance contains two files with the same logic, one in es6 module style (main.es.js), and a commonjs one too (main.js). Webpack, annoyingly, picks the es module unless you specify otherwise in your config.

What geojson-path-finder can do in order to work with Webpack is to explicitly say that it wants to use the commonjs version, with:

var distance = require('@turf/distance/main') // use /main to ensure the commonjs file is used

Adding .default as suggested above does something slightly different, and uses the es6 version but targets the default export.

P.S. there's a much more involved fix of making this package ES6 and compiling it to ES5 with Webpack.

perliedman commented 5 years ago

https://twitter.com/tmcw/status/1091023428780470273 ¯_(ツ)_/¯

Having said that - if you come up with a non-intrusive way to improve the situation for webpack users, feel free to make a PR. This is not a priority for me personally, though, and I do not think it is a bug in geojson-path-finder.

vincentfretin commented 5 years ago

You should be able to support both es6 and cjs modules by doing this I guess:

var distance = require('@turf/distance/main')
distance = 'default' in distance ? distance['default'] : distance;

Could someone test this and maybe create a PR?

davidgilbertson commented 5 years ago

Thanks for the reply @perliedman - you're right, it's certainly not the fault of geojson-path-finder, you're just collateral damage.

I'm happy to do a PR. I think

var distance = require('@turf/distance/main')

Is the most straightforward approach.

@vincentfretin is there a benefit to having geojson-path-finder use the ES6 version of @turf/distance?

qunabu commented 5 years ago

I can confirm that changing to

 distance = require('@turf/distance').default,

does work with react and react-create-app as we used this in one project https://niezapomniani.org/ so

const distance = require('@turf/distance/main');
distance = 'default' in distance ? distance['default'] : distance;

would be great solution.

vincentfretin commented 5 years ago

@davidgilbertson @turf/distance is such a small module, so it doesn't really give you a benefit if you use ES6 module vs commonjs module. Webpack by default gives priority to ES6 modules (the module key in package.json) over commonjs (the main key in package.json). Webpack can remove some code so reducing your bundle size by tree shaking in some conditions if the dependency expose ES6 modules with sideEffects : false and if you use @babel/preset-env with modules: false (more details here https://webpack.js.org/guides/tree-shaking/)

perliedman commented 5 years ago

It appears updating @turf/distance to latest version breaks even with a normal Node environment without webpacks/imports, so I have simply switched to importing like suggested above: distance = require('@turf/distance').default. Please notify if this cause issues for anyone.