TypeError: Cannot read property 'lat' of undefined

[whyvez]

I'm using react-leaflet-markercluster with with children markers and getting TypeError: Cannot read property 'lat' of undefined when I update the markers. From the readme it looks like this is not fully supported yet. Could you expand on this?

I think the cause might be how keys are applied to the cloned marker element.

From my understanding React uses the key to determine on update if the item is new or an updated earlier one and the react docs advise against using index as a key.

https://facebook.github.io/react/docs/lists-and-keys.html

Keys help React identify which items have changed, are added, or are removed. Keys should be given to the elements inside the array to give the elements a stable identity:

When you don't have stable IDs for rendered items, you may use the item index as a key as a last resort

I'm trying to debug this but having issues building the project. At first glance I thought it was the node version. I went from v8 to v4 but still not building. Could you let me know which node version the project targets. Might also be worth adding the engines property in the package.json with the targeted node. I can also create a seperate issue for this specifically if you think it might be best.

[yuzhva]

@whyvez I'll try to test react-leaflet markers update during this week.

You could be right because there is really simple logic for rendering react-leaflet Marker component.

[whyvez]

Perfect. Could use the marker id. What version of node should I use to build?

[skipjack]

I'm seeing this issue as well and would love to help push a fix forward. @whyvez @YUzhva did either of you make any progress on a resolution?

I'm going to pull this project locally to npm link it and see if I can figure something out. I'll use what's been mentioned above as a starting point but if someone has already made progress that would be helpful to know.

[skipjack]

@whyvez I was able to clone and build though I did have to make a few tweaks. I'm on Node v6.9.4 and NPM v3.10.10. Let me know if you're still interested in this as I could share a fork.

@YUzhva there are probably a few things that could be simplified in relation to config and deployment:

1. Typically it's not recommended to ship to npm with CSS require baked in (related to #40). This means all consumers must be using webpack and have a css-loader configured. Instead users can just decide whether or not to include the CSS as they see fit.
2. Also, I would recommend just using webpack over both webpack and gulp. Everything this package does with gulp could also be accomplished with webpack. Having two build systems can definitely make things a bit tougher to understand and I think webpack fits the usage here better.

Anyway, just thought I'd mention the things I noticed while going through this as some clean up might make the package easier to maintain/contribute to.

[yuzhva]

@skipjack your advises is really nice.

It's been a while since I create that wrapper, so I learned new things, but I didn't have enough time to improve them(

[skipjack]

your advises is really nice.

Happy you found it helpful!

It's been a while since I create that wrapper, so I learned new things, but I didn't have enough time to improve them

Yeah I totally get that, it's just the nature of open source work, right? 😜

So after digging into this for a few hours yesterday, here's a summary of what I found...

Actual Error

@whyvez I'm not sure what the stack trace was for you, however the core of it for me was:

project | @ | leaflet-src.js:1612 latLngToPoint | @ | leaflet-src.js:1452 project | @ | leaflet-src.js:3834 _removeFromGridUnclustered | @ | leaflet.markercluster.js:6 _removeLayer | @ | leaflet.markercluster.js:6 removeLayer | @ | leaflet.markercluster.js:6 _moveChild | @ | leaflet.markercluster.js:6 _childMarkerMoved | @ | leaflet.markercluster.js:6 fire | @ | leaflet-src.js:588 setLatLng | @ | leaflet-src.js:7654 updateLeafletElement | @ | CircleMarker.js:31 componentDidUpdate | @ | MapLayer.js:50 componentDidUpdate | @ | Path.js:33 ...

So, after digging into various fixes within this library for a while with no luck, I decided to come back to the highlighted lines. Here's what I found:

    if (toProps.center !== fromProps.center) {
      this.leafletElement.setLatLng(toProps.center)
    }

These lines within CircleMarker are the "source" of the error. Seeing that the center prop is either an array, object, or LatLng instance -- the equality check doesn't actually work (though I'm guessing it's dependent on object identity or something). Those two arrays were actually equal in my case and when the check is replaced with a latLng(...).equals call...

if ( latLng(toProps.center).equals(fromProps.center) ) {

The setLatLng code is never executed, thus resolving the error. The thing I still haven't been able to figure out is why the setLatLng call fails in the first place. The toProps.center value is defined when it's called so something else must be going on behind the scenes. Maybe we can ping the leaflet and react-leaflet maintainers to get a better idea.

Thoughts on this Package

So, while the bug may not be related to this package, and aside from what I mentioned above re CSS and build process, there were some other things I noticed about the base component.

First, I don't think you should support markers and markersOptions props. They increase the complexity of your component significantly and aren't available as options in the base leaflet.markercluster package as far as I can tell. Rather, I think you should just support markers as children as it's fairly painless for users to consume it this way, e.g. instead of markers={ points } they could just write:

<MarkerClusterGroup ...>
  { points.map(point => <Marker position={ point } />) }
</MarkerClusterGroup>

This would be a breaking change obviously, but I think it's a worthwhile one. Which leads me to my second point. With markers and markersOptions removed, I was able to simplify this component a great deal:

import React from 'react';
import PropTypes from 'prop-types';

import L from 'leaflet'
import 'leaflet.markercluster';

import { MapLayer } from 'react-leaflet';

import './style.css';

export default class MarkerClusterGroup extends MapLayer {
  static propTypes = {
    children: PropTypes.node,
    options: PropTypes.object,
    wrapperOptions: PropTypes.object
  }

  static defaultProps = {
    wrapperOptions: {}
  }

  static childContextTypes = {
    layerContainer: PropTypes.object
  }

  getChildContext() {
    return {
      layerContainer: this.leafletElement
    }
  }

  componentWillMount() {
    super.componentWillMount()

    let { enableDefaultStyle, disableDefaultAnimation } = this.props.wrapperOptions
    
    if ( enableDefaultStyle ) this.context.map._container.className += ' marker-cluster-styled'
    if ( !disableDefaultAnimation ) this.context.map._container.className += ' marker-cluster-animated'
  }

  createLeafletElement(props) {
    return new L.markerClusterGroup(props.options)
  }
}

This offsets most of the work on child elements calling addLayer on the marker container and avoids cloning or any react element generation which feels a bit funny imho. Also if you rethink the CSS stuff and offset that to consumers as well the whole componentWillMount method goes away.

Anyway, I just figured I'd share what I'd found. I'm still fairly new to react-leaflet but it seems this direction would be closer to what's recommended in their guide. As mentioned above, I'd be happy to push my changes to a fork or branch (if given permissions). I don't have enough bandwidth to take this all the way on my own atm, but I'd be happy to advise someone who does. I maintain the new webpack documentation so I'd definitely be able able to help guide the build process refactoring if desired.

For now, I'm just going to create a custom CircleMarker instance with that fix.

[yuzhva]

@skipjack Your solution is awesome. I was "playing" with it during those weekends and there everything was awesome!

So, I decided to implement it and publish to npm with test coverage... (you know, all those cool labels at repository)

But there were issues... I supposed to finish with your solution and test coverage during the next weekends. Hope to publish it (=

[skipjack]

@YUzhva great! Feel free to ping me if you need anything clarified or want someone to test the update.

[mmichelli]

Getting the same issues with the cluster children, "Uncaught TypeError: Cannot read property '0' of undefined", after updating a cluster.

[skipjack]

@YUzhva loving how simple this codebase has become. I'm about to try v2 (i.e. react-leaflet-markercluster@next) since I just upgraded to react-leaflet@2.

[jwmann]

I'm still getting this issue, specifically with CircleMarkers as well. @skipjack Can you explain what you did to workaround this?

If I change the CircleMarker to a regular Marker, I don't get this issue.

Using react-leaflet-markercluster@next and react-leaflet@2

[skipjack]

@jwmann see my comment above. I modified the equality check in CircleMarker.

[amakaa]

@skipjack so you have to make the change inside the node_modules?

[Falkyouall]

@skipjack Thanks for your work, i edited this comment 5 times until i made it work, revoked all my snippets. Still theres my question why your equal snippet checks for equality as the original checks for inequality ?

if (!(latLng(toProps.center).equals(fromProps.center)))

And can we fix this @PaulLeCam?

for now i made a npm patch that fixes that issue in my repo & for all branches, and also within my CI install. i can recommend. Everyone waiting for this fix you can support my merge request

[Falkyouall]

Unfortunately my PR has not been accepted. I don't know why exactly the !== method makes more sense than the equals method with is catching any non usable values and proceed with the computation. I would love to resolve this, asap.

[yuzhva]

@Falkyouall did you try to upgrade your dependencies to that latest v2.x?

yarn add react-leaflet-markercluster@next

[aqueiros]

I confirm that this was happening to me when using CircleMarker inside MarkerClusterGroup I found the fix for this on @Nadhum's comment here https://github.com/Leaflet/Leaflet/issues/6257

and passing not only center but also position into the CircleMarker

[Falkyouall]

@YUzhva Hey, sorry for late answer, i should subscribe this thread. And Yes i'm using the latest react-leaflet version

"react-leaflet": "^2.4.0", "react-leaflet-markercluster": "^2.0.0-rc3"

I still have a working npm-patch script that fixes the issue by npm postinstall with the above solution.

[elesdoar]

Hi, maybe you should to review the dependency tree, I had the same problem, from a library loads leaflet from its node_modules folder, also, my app was loading leaflet from its node_modules folder, Leaflet did a instanceOf comparison. Some as:

https://github.com/Leaflet/Leaflet/blob/master/src/geo/LatLngBounds.js#L246

export function toLatLngBounds(a, b) {
	if (a instanceof LatLngBounds) {
		return a;
	}
	return new LatLngBounds(a, b);
}

For some reason, a is a instance of LatLngBounds loaded from my app node_modules folder leaflet loaded and b is null, but toLatLngBounds is from a library node_modules folder, of course, a instanceof LatLngBounds returns false, its going to return new LatLngBounds(a, b); line, and returns a LatLngBounds empty.

I was finally able to fix the error, setting the leaflet load on config-overrides.js

addWebpackAlias({
    ...,
    'leaflet': path.resolve('node_modules/leaflet')
})

[Maarcel]

@elesdoar can you describe your solution in more detail? I'm using plain leaflet without react. How can i setup this override in a simple TypeScript with Webpack Application?

My Problem in more detail: Leaflet Markercluster TypeError: Cannot read properties of undefined (reading 'lat')

[abawchen]

@elesdoar : thanks, you save my day :tada: