GuillaumeLeclerc / vue-google-maps

Google maps component for vue with 2-way data binding
560 stars 653 forks source link

VueJs 2 compatibility #112

Closed khenam closed 7 years ago

khenam commented 7 years ago

All components are updated to be compatible with VueJs 2.0 All Properties with twoWay configuration are merged in a single object Objects to represent this components was created and com be used to create then

xkjyeah commented 7 years ago

First impressions:

  1. It almost feels like a new package, potentially breaking backward compatibility
  2. Shouldn't the eventHub be limited to the components of the same map?
  3. I'm not so sure about wrapping all the object options into an opaque "Vue*Object". Seems to defeat the purpose of having a declarative syntax. What was the rationale for that?
khenam commented 7 years ago

Hi, Thanks for answering,

  1. Yes, unfortunately with the new changes made by the new version in my initial analysis would break compatibility. I would be grateful if you could show me a way which did not lose backward compatibility.
  2. You are correct. I worried more to modify the minimum possible the existing logic, in order to facilitate correction. Because of this I did not bother with the use of more than one map on the same page. The best way to use this integration is to put the shared elements in an object of Vuex and use it in the components. I accept suggestions to make it more secure, without changing the current logic.
  3. Sorry about that, I ended up getting the habit of seeking traceability in my code, so I created these objects. They are not required to create components. They are only an aid for those who want your IDE or your code analysis tools to understand which are traceable information.

Thanks again and waiting recommendations.

xkjyeah commented 7 years ago

My suggestions:

  1. You raise a good point about the IDE. However you should keep the old properties. Instead of making them properties of a separate object. This will maximize compatibility. Unfortunately, due to how Vue 1.x and 2.x use their own implementation of classes, I doubt that there will be an easy way in the short run of integrating type checking into it. There might be a solution.
  2. I believe that for vue-google-maps, it is entirely clear what the following markup means:
<gmap-map>
    <gmap-marker></gmap-marker>
</gmap-map>

and in fact I believe we want it to look that way, without having to deal with a common event bus. After all a <gmap-marker> will look very meaningless if it is not declared within a map object.

Hence, in lieu of Vue 1.x-style events or the Vue 2.x-style event bus, I think it is alright to disregard the strict independence of parent/children components in our case.

Which means,

  1. Define the event bus as a property of the map component
  2. Map markers and shapes can search their ancestors for their parents, e.g.
var mapAncestor = this.$parent
while (!(mapAncestor instanceof Map)) {
    mapAncestor = mapAncestor.$parent
    if (mapAncestor.$parent === null) {
        throw new Error("This component must be a child of a map!")
    }
}
  1. This should be done within the abstract MapComponent class (maybe it should be renamed to MapElement) so that you can avoid code duplication.
  2. The if (this.destroyed) ... checks should also be moved into MapComponent or DeferredReady.
khenam commented 7 years ago

I think I found an intermediate solution to the issue of backward compatibility. Computed field reading the property on get and calling event on set. I modified the map to work this way. I'm modifying the others. After that I will modify the way of iteration of the map descendants as you suggest, it's really better.

<google-map
    :center = "center"
    :zoom="zoom"
    @center_changed = "centerChanged"
></google-map>
khenam commented 7 years ago

Hi @xkjyeah, I did your recommendations, and remove the use of VueObjects. About TS i love it, but in this moment will be hard make this change because my deadline. In an other moment will be nice make this and add automated tests too. Waiting the analyses and thanks again your attention.

yisibl commented 7 years ago

@xkjyeah Do you want fix this?

// FIXME: In version 1, we preserved the center of the map

https://github.com/xkjyeah/vue-google-maps/blob/vue2/src/components/map.vue#L87

xkjyeah commented 7 years ago

@yisibl The way I envision vue-google-maps is for it to be a thin layer between Vue and Google Maps. This helps us avoid incompatibilities when either library is updated, and reduces number of surprises when users go from plain Google Maps to Vue.

Therefore, rather than change the semantics of resize(), I would rather there be an alternative method (with a name like resizePreserveCenter()) which does the centre-preserving resize. Would you like to make a PR?

(In any case, you will have to make a PR on vue2-google-maps, not on this repository which is vue-google-maps)

yisibl commented 7 years ago

@xkjyeah Look forward to your update :joy:

khenam commented 7 years ago

No problem, i think change to resizePreserveCenter a good idea. But i don't know how make a PR to vue2-google-maps, maybe i need some help.

xkjyeah commented 7 years ago

@yisibl I just published the latest version (0.3.0) which supports resizePreserveCenter(). Get it with npm install vue2-google-maps@latest.

If you have any further feature requests, do file an issue there. I do not intend to make a pull request to vue-google-maps because there is no way to simultaneously maintain support for both vue1 and vue2 in the same package, and I do not want to break vue-google-maps's compatibility with Vue 1.x. (somewhere out there some user's application, possibly my own, is going to break if we do so).

@khenam Sorry about my parallel version. I saw your pull request only when I had already started my own port to Vue 2.x and had to make the port quickly for my own work.

khenam commented 7 years ago

Sorry @xkjyeah, I have difficult to compatibilize our branches. You change some concepts. Its better create a new branch in your repo and make this together? or wait i merge and send to you.

xkjyeah commented 7 years ago

Do you know which features are missing in my repo? It might be easier to just merge in features that are missing / different.

I've published the API on Github. You can compare it with yours to see if anything is missing.

On Nov 21, 2016 19:43, "Ronald Lima" notifications@github.com wrote:

Sorry @xkjyeah https://github.com/xkjyeah, I have difficult to compatibilize our branches. You change some concepts. Its better create a new branch in your repo and make this together? or wait i merge and send to you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GuillaumeLeclerc/vue-google-maps/pull/112#issuecomment-261915515, or mute the thread https://github.com/notifications/unsubscribe-auth/ACiTR0nNM2tI1z-hhRpz1kVWjmxOWA8-ks5rAYPvgaJpZM4Kkh_i .

yisibl commented 7 years ago

@xkjyeah Nice work, thank you 👻

khenam commented 7 years ago

Tranfer PR to @xkjyeah Repo https://github.com/xkjyeah/vue-google-maps/pull/2