Changing/updating properties of components is prohibitively difficult

[ericsoco]

I ran into a situation using GeoJson in which I wanted to highlight a selected GeoJson layer on click, and was trying to do so by adding/removing a class. However, I wasn't able to make the changes happen in React's render cycle, and instead had to manually select elements (with document.querySelector) and add/remove classes.

I believe this is due to the way Leaflet manipulates the DOM on its own, and how react-leaflet produces dummy <div>s for React. I was conditionally setting classNames, but React's DOM diff was looking at those <div>s instead of of the <path>s to which react-leaflet was applying the actual classes.

This resulted in no change:

renderGeoJsonLayers () {
    let layers = [];
    features.forEach(thing => {
        className = 'foo';
        if (feature.properties.id === selectedId) {
            className += ' selected-foo';
        }

        layers.push(<GeoJson className={ className } key={ 'foo-' + feature.properties.id } data={ feature } />);
    });

    return layers;
}

Instead, I had to add an additional className that contains thing.properties.id and use that to form a selector to remove 'selected-foo' from the previously-selected element, when a new element is selected.

I'm not sure how react-leaflet might solve this problem -- perhaps the classNames can be applied to the dummy <div>s as well as the rendered <path> elements, and the React DOM diff will pick up the changes? Not sure if the changes would propagate through to the <path> elements though, seems like that would still be up to react-leaflet / Leaflet...

I also don't know if this is something that needs to be solved for arbitrary attributes, or just for className / class. Haven't thought about it that deeply.

[PaulLeCam]

Hi,

I'm not sure I understand the problem here, is it with React's diffing or applying the properties?

The dummy <div>s are simply used as containers for children elements, but they are not actually displayed so they shouldn't be used for styling.
All the rendering is handled by Leaflet but the diffing in the tree is made by React, so the best way to force the update of a layer is to change its key property, so that React simply replaces it.

[ericsoco]

Interesting tip on changing the key attribute. I'll give that a shot.

I think you do understand the problem, based on your suggestion -- updating attributes of a react-leaflet component (in subsequent render() calls) does not actually update the component. (I saw this first with adding/removing a class in className, and I think I also saw that changes to a GeoJson's data didn't change the <path>'s d attribute.

I'm short on time now but can try to boil this down to a working repro if that is useful.

[PaulLeCam]

The GeoJson data prop is not dynamic, so if it changes you should change the key as well to make sure it's properly updated.

The path options should be updated though, here is the logic handling this: https://github.com/PaulLeCam/react-leaflet/blob/master/src/Path.js#L32
If it doesn't update when one of the Path options prop is changed clearly it's a bug, I'll have a look at it but I don't have so much time at the moment so could take a few days. In the meantime if you can provide a working repro it would certainly help 👍

[luqmaan]

changes to a GeoJson's data didn't change the <path>'s d attribute.

Heres a naive implementation of an updatable GeoJson component https://github.com/open-austin/austingreenmap/blob/9b0d9ad5ddc245c07c63ab7d2997e74328d73259/client/js/components/GeoJsonUpdatable.jsx.

[ericsoco]

@luqmaan that's essentially what I ended up doing, though I did not encapsulate it into a component like you did. @PaulLeCam I'm under the gun right now and can't put together a repro just yet. Will do so as soon as I'm able.

[PaulLeCam]

@luqmaan I wonder what is the advantage of this solution rather than changing the key prop?

I haven't done any test, I just assumed having React remove the node and Leaflet create a new one would be less expensive than having Leaflet remove its layers, add new ones, and apply the styles, but maybe I'm wrong.

[ericsoco]

@PaulLeCam that's the technique @mourner recommends, in absence of any formal way to update data: Leaflet/Leaflet#1416 (comment)

[PaulLeCam]

@ericsoco Thanks for the link, I'll add this to the next release to handle dynamic changes of the data without needing to change the key.

I haven't had time to investigate your issue yet, but I set a JSFiddle up, if you could please use it to try to reproduce the issue when you have some time that would help!

[ericsoco]

Thanks @PaulLeCam -- still working toward a deadline (two projects converging...) so will be a bit but will come back to this as soon as I'm able!

[hannesj]

Also related to changing classnames is Leaflet/Leaflet#2662

[PaulLeCam]

Thanks for this link.
Clearly if Leaflet doesn't support setting className it won't be supported in this lib either.

[Spaxe]

I ran into this exact issue, thanks @PaulLeCam for the suggestion about updating the key. That was more difficult to debug than it should have been.