Skip to content

Conversation

naderio
Copy link
Contributor

@naderio naderio commented Apr 1, 2018

This PR does the following:

  • remove unused parentMapObject state
  • export findRealParent and propsBinder utils to be used in plugins
  • fix setting up latLng within LMarker which caused issues with vue2-leaflet-markercluster

@KoRiGaN
Copy link
Collaborator

KoRiGaN commented Apr 1, 2018

Hi @naderio,

Thanks for this pull request.

  • remove unused parentMapObject state
    => Ok
  • export findRealParent and propsBinder utils to be used in plugins
    => Ok
  • fix setting up latLng within LMarker which caused issues with vue2-leaflet-markercluster
    => Please explain what is the issue in vue2-leaflet and how the fix can be tested

Your pull request also changes the package version. Please don't do it as it should be done on its own commit with a tag that triggers the build and publish process.
Your pull request also changes the example package by adding vue2-leaflet as a file reference instead of npm link.

Mickael

@naderio
Copy link
Contributor Author

naderio commented Apr 2, 2018

Hi @KoRiGaN,

Thank you for the follow up.

I rolled back the package version and and example dependency.

Regarding LMarker.latLng, they way it is set combined with $watch within propsBinder is causing a number of issues with Leaflet.markercluster, mainly due to animation (thus setting latLng too many times).

Stack trace is similar but not limited to the following and markers are either shown the first time or not shown at all:

vue.runtime.esm.js?a427:587 [Vue warn]: Error in callback for watcher "latLng": "TypeError: Cannot read property '_zoom' of undefined"

found in

---> <LMarker>
       <VMarkerCluster> at /opt/Workspace/contrib/vue2-leaflet-markercluster/Vue2LeafletMarkercluster.vue
         <LMap>
           <Example> at /opt/Workspace/contrib/vue2-leaflet-markercluster/example.vue
             <Root>

TypeError: Cannot read property '_zoom' of undefined
    at NewClass.addLayer (leaflet.markercluster-src.js?7b9c:135)
    at NewClass._moveChild (leaflet.markercluster-src.js?7b9c:718)
    at NewClass._childMarkerMoved (leaflet.markercluster-src.js?7b9c:705)
    at NewClass.fire (leaflet-src.js?6144:593)
    at NewClass.setLatLng (leaflet-src.js?6144:7405)
    at VueComponent.n.(:4000/anonymous function).custom.setOptions.t.$watch.deep (webpack-internal:///80:1:2003)
    at Watcher.run (vue.runtime.esm.js?a427:3229)
    at flushSchedulerQueue (vue.runtime.esm.js?a427:2977)
    at Array.eval (vue.runtime.esm.js?a427:1833)
    at flushCallbacks (vue.runtime.esm.js?a427:1754)

Thanks.

@naderio
Copy link
Contributor Author

naderio commented Apr 2, 2018

@KoRiGaN Steps to reproduce the issue:

@KoRiGaN
Copy link
Collaborator

KoRiGaN commented Apr 2, 2018

@naderio,

Thanks for the reply.
I will try to have a look at it and see if this change doesn't have drawbacks.
If everything is fine I will merge this.

Micka

@mits87
Copy link

mits87 commented Apr 10, 2018

When can we expect a merge of this?

@sangdth
Copy link

sangdth commented Apr 10, 2018

😆 I'm still using this in my package.json:
"vue2-leaflet": "github:KoRiGaN/Vue2Leaflet#7b0406a8a4af21d83517f0e576dd04338b0dfc2f"

@jperelli
Copy link
Contributor

Hi @KoRiGaN I tested it and I think this is safe to be merged. Please let us know if you need any help!

@KoRiGaN KoRiGaN merged commit 339f2a0 into vue-leaflet:master Apr 13, 2018
@KoRiGaN
Copy link
Collaborator

KoRiGaN commented Apr 13, 2018

Hi @naderio, @mits87, @jperelli,

I've merged this PR into master, but it wasn't working correctly when lat or lng was updated but not the latLng object itself.
So I've added a new commit fixing this PR and pushed it in version 1.0.2.

I hope it doesn't unfix your fix.
I couldn't test it because I have no way to reproduce the original issue easily.

Let me know how things are going.

@naderio
Copy link
Contributor Author

naderio commented Apr 13, 2018

Thank you @KoRiGaN.
It seems to be working fine.

@sangdth
Copy link

sangdth commented Apr 13, 2018

I will try to test on weekend, then report here. Thank you all for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants