EastCoastGreenwayAlliance / ecg-map

Interactive map and trip planner for the ECGA
https://map.greenway.org
7 stars 0 forks source link

error in location search on "Start Route" #92

Closed danrademacher closed 5 years ago

danrademacher commented 5 years ago

The system seems to hang on Start Route: image

  1. Go to a route like this: https://map.greenway.org/?loc=7,43.16112,-69.65881&route=42.36057,-71.05323,44.80088,-68.77076

  2. Click Start Route and app hangs with the error sown above:

    Uncaught TypeError: Cannot read property 'properties' of null
    at b.<anonymous> (LeafletMap.jsx:517)
    at b.fireEvent (cartodb.js:7)
    at b._handleGeolocationResponse (cartodb.js:11)
    at cartodb.js:7

    the LeafletMap.jsx line is question is here: https://github.com/EastCoastGreenwayAlliance/ecg-map/blob/master/client/src/components/LeafletMap.jsx#L517

Maybe some downstream effect of the pg-router?

clhenrick commented 5 years ago

don't think this is the cause but probably not helping that self is not defined in the constructor of the LeafletMap component. So setting self.locateMe = null isn't doing anything and if something later references the locateMe property on the class it will be undefined instead of null.

clhenrick commented 5 years ago

@gregallensworth looks like the problem is in this block of code on lines 496 - 511.

Line 497 creates a variable that references a single lat, lon pair, but the for loop below it makes it seem like this should instead be an array. As a result the value of closest.segement remains null so that when code further below attempts to reference the properties property of closest.segment it throws an error.

A similar block of code is on lines 695 to 709 that appears to have the same problem.

gregallensworth commented 5 years ago

Indeed; though that typo doesn't seem to be the more significant one.

Reference: LeafletMap.jsx initMap() around line 269.

this.locateMe will only be "filled in" is isMobile and will remain null if isMobile is false.

isMobile is defined when ActiveTurningReadoutConnected is initialized, as ! browser.greaterThan.medium (according to redux-responsive this is 1000px)

That is to say:

So, a workaround could be to always instantiate self.locateMe = L.control.locate(...) and accept that on a large screen where we never offer turn-by-turn, that this is useless but harmless.

gregallensworth commented 5 years ago

I think we're seeing two different problems here: a) screen size change can show Start Route button without a locateMe to back it; b) the real-time tracking then has an issue which Chris describes.

gregallensworth commented 5 years ago

Both items fixed. Branch gda-issue92

Good eye, Chris, on that extra [0]

gregallensworth commented 5 years ago

Merged and deployed, and confirmed that this fixes both noted items.

clhenrick commented 5 years ago

@gregallensworth no problem!

So, a workaround could be to always instantiate self.locateMe = L.control.locate(...) and accept that on a large screen where we never offer turn-by-turn, that this is useless but harmless.

Another work around could be to extract the locateMe init to a separate class method, and then call that method when isMobile changes in componentWillReceiveProps.

The dynamic import that loads the L.Control.Locate script is a nice optimization that spares the user some un-necessary downloaded code, though I'm not sure it matters much on desktop as someone is more likely to have a better connection there than on mobile.

A better use case of the dynamic import would probably be to wait until the user clicks the "start route" button, which could call an initLocateMe method, and then do the rest. That way the code wouldn't be requested and parsed in the browser until it was actually needed.

Otherwise you may as well get rid of the dynamic import as it's not serving any real benefit AFAIK.