smeijer / leaflet-geosearch

A geocoding/address-lookup library supporting various api providers.
https://smeijer.github.io/leaflet-geosearch/
MIT License
1.02k stars 270 forks source link

Broken in leaflet 1.8.0 #323

Closed jerome-lemaire closed 2 years ago

jerome-lemaire commented 2 years ago

Hi !

The plugin does not work anymore with Leaflet 1.8.0 but ok with Lealfet 1.7.1

minmal code : https://codesandbox.io/s/frosty-tamas-yevb21?file=/src/index.js

Uncaught TypeError: Cannot read properties of undefined (reading 'indexOf') at NewClass.addTo (leaflet-src.js:4789:1)

unzsnu commented 2 years ago

I fixed this error setting the position

 const searchControl = new GeoSearchControl({
      position: 'topright', /* <---- add this, options: 'topleft', 'topright', 'bottomleft', 'bottomright' */
      provider: props.provider,
      style: props.controlStyle,
      ...props
    });

Position has not been set yet, so getPosition() returns undefined and then, checking the position, it breaks

thecodemonk commented 2 years ago

Using the sample site posted above as a test, the search doesn't actually seem to be working. I am noticing the same thing and we use MapBoxProvider. Downgrading to 1.7.1 fixed it.

ewlarson commented 2 years ago

I'm also seeing that search doesn't work with Leaflet v1.8.0. @unzsnu's suggestion does allow the control to render, but search isn't operable.

brbrbr commented 2 years ago

Appears that the default options are not initialized correctly.

I added the lot (extending the fix from unzsnu

  var searchControl = new GeoSearch.GeoSearchControl({
          provider: provider,
          position: 'topleft',
          showMarker: true,
          marker: {
            icon: get_icon('roos'),
            cat: 'roos',
            draggable: true,
          },
          maxMarker: 1,
          autoClose: true,
          autoComplete: true,
          retainZoomLevel: true,
          maxSuggestions: 5,
          keepResult: true,
          resultFormat: function(t) {
            return "" + t.result.label;
          },
          updateMap: !0

        });
gregorbg commented 2 years ago

I tried to look into this, and it seems that the extend call at https://github.com/smeijer/leaflet-geosearch/blob/develop/src/SearchControl.ts#L472 was changed in leaflet as part of the 1.8.0 release: https://github.com/Leaflet/Leaflet/commit/7b15104f2fc5e16fb23d5e13468c5df558cbf006.

Now, the extended class does not have the default options anymore (which is why the original indexOf error occurs), nor does it overwrite the showResult function properly (which is why search isn't working even if you add certain option props)

Maybe someone who is more knowledgeable than me in JS inheritance and prototypes can use this information to figure out a proper solution :)

Edit:There's also this: https://github.com/Leaflet/Leaflet/commit/a340c086c2c5b2f8fcf95da0765a72566bb54d2a which supposedly breaks overriding the default properties

Edit 2: It really seems to be https://github.com/Leaflet/Leaflet/commit/a340c086c2c5b2f8fcf95da0765a72566bb54d2a. Reverting that one-line change in Leaflet fixes everything in my local project.

brbrbr commented 2 years ago

I have no clue what all the typescript and prototyping stuff does. However, replacing the ... operator with the native L.setOptions resolves the issues for me.

@@ -164,8 +164,8 @@ const Control: SearchControl = {
     }

     // merge given options with control defaults
-    this.options = { ...this.options, ...options };
-    this.classNames = { ...this.classNames, ...options.classNames };
+    L.setOptions(this,options)
+    this.classNames=this.options.classNames

     this.markers = new L.FeatureGroup();
     this.classNames.container += ` leaflet-geosearch-${this.options.style}`;
smeijer commented 2 years ago

Should be fixed in 3.6.1. Thanks to @vkunz (#327)

brbrbr commented 2 years ago

Fixes the issue.

In the mentioned PR on leaflet, a commenter depicts that initialize should call setOptions:

https://github.com/Leaflet/Leaflet/pull/7459#pullrequestreview-586639979