diegoazh / gmap-vue

A wrapper component for consuming Google Maps API built on top of Vue. Fork of the popular vue-google-maps plugin.
https://diegoazh.github.io/gmap-vue/
173 stars 51 forks source link

Bug: gmap-vue 2.0 latest release package is broken #226

Closed davydnorris closed 2 years ago

davydnorris commented 2 years ago

Describe the bug

Have just updated a working nuxt project to gmap-vue 2.0, and now running it causes error:

Failed to compile with 1 errors
This dependency was not found:
gmap-vue in ./plugins/gmap-vue.js

To install it, you can run: npm install --save gmap-vue    

To reproduce

Steps to reproduce the behavior:

  1. Create a nuxt project
  2. Set up plugin/gmap-vue.js as per documentation:
    
    import Vue from 'vue';
    import * as GmapVue from 'gmap-vue';

export default function ({ $config }) { Vue.use(GmapVue, { load: { key: $config.googleAPIKey, libraries: 'places,visualization,drawing' }, installComponents: true }); };


3. npm run dev
4. See error log

## Expected behavior

gmap-vue package is found and loads as before

## Current behavior

gmap-vue is not loading because the package cannot be found

## Screenshots

N/A

## Desktop (please complete the following information)

 - OS: Windows 10

## Smartphone (please complete the following information)

N/A

## Additional context

Have checked the node_modules directory and gmap-vue is there
Have removed node_modules completely and done a full reinstall and problem is still there
Have reverted to earlier package and code runs fine

## Versions

- Node: 16.13.0
- NPM: 8.1.4
- Yarn: N/A

## Package manager

- [X] NPM
- [ ] Yarn

## Plugin version
- [X] 2.0
- [ ] 1.4.x
- [ ] 1.2.x
- [ ] 1.0.x
Andrei1524 commented 2 years ago

i have the same problem

diegoazh commented 2 years ago

Hi, @davydnorris I am glad to hear from you, and thanks @Andrei1524 for joining the discussion. I think the error is caused because I change the way in that the plugin makes its exports.

Now I export by default the install function and two objects helpers and components you don't need to do this

import * as GmapVue from 'gmap-vue'

any more, now you can do this

import GmapVue from 'gmap-vue'

following the ES6 spec form import/export.

Check if that solves the problem, I think I miss to update that in the guides, but in the API doc should be documented.

davydnorris commented 2 years ago

Thanks @diegoazh I'll give that a try - and you're correct, the documentation still shows the old way.

I was also going to drop you a line as I was looking through the drawing manager changes and noticed a few things:

diegoazh commented 2 years ago

I found the error, something happened on the CI/CD process of GitHub Actions the plugin was packaged inside of the dist folder you need to import it in the following way

import GampVue from 'gmap-vue/dist/gmap-vue';

the dist folder shouldn't be there, what do you think guys I add this note to documentations or I deprecate this version and land a new one?

On the other hand, answering your questions

davydnorris commented 2 years ago

No, that's quite non-standard in terms of packages on npm - I would deprecate and push a new one

davydnorris commented 2 years ago
* Yes I copy the array, to avoid mutations from the component, but you have all the shapes because on every
update the component is emitting the event `update:shapes` to send to the parent a copy with the new shapes
added to the array, I think that was your question right?

The thing with the drawing manager is that it's an editor and so you really want the mutation - you want two way binding. The shape objects are also quite big and if there are a lot of them then passing the entire array back on every single update will be a performance nightmare.

Also once the shape is drawn, you have no way to change it except by altering its properties directly, and you can't do that if it's hidden inside the component - let's say that someone wants to implement a colour palette as part of their drawing surface, and then allow the user to shift click or rubber band the shapes they have drawn and change their colour. This isn't possible in your current implementation because the shapes that are actually attached to the map are hidden inside the component. It's much better to let the user have control over the shape array and copy it if they want to keep a backup before invoking the drawing manager

davydnorris commented 2 years ago
* On points 3 and 4 I'm not sure to understand well the case that you mention, for colours and other 
properties you can send it inside of the options property and it should work but maybe in these two last
questions should more easy to make a small repo to reproduce the issues and analyse the cases.

Have a look at code starting from this line

The setSelection and clearSelection functions use hard coded colours for the fill and border. What should really be happening is that we should have a prop to let the user specify border and fill colour for selected shapes, and we should return the shape back to either its original border and fill colour, or the border and fill colours specified in the shape options prop when it's deselected.

When I wrote this code, I made sure the hard coded values matched the options, and so the example code all looks fine. But if you change the border and fill colours in the sample code to, say, light red, then select and deselect a shape, you'll see it doesn't go back to red, it goes light grey because that's the hard coded colour in clearSelection

CristianDeluxe commented 2 years ago

Same issue here

diegoazh commented 2 years ago
* Yes I copy the array, to avoid mutations from the component, but you have all the shapes because on every
update the component is emitting the event `update:shapes` to send to the parent a copy with the new shapes
added to the array, I think that was your question right?

The thing with the drawing manager is that it's an editor and so you really want the mutation - you want two way binding. The shape objects are also quite big and if there are a lot of them then passing the entire array back on every single update will be a performance nightmare.

Also once the shape is drawn, you have no way to change it except by altering its properties directly, and you can't do that if it's hidden inside the component - let's say that someone wants to implement a colour palette as part of their drawing surface, and then allow the user to shift click or rubber band the shapes they have drawn and change their colour. This isn't possible in your current implementation because the shapes that are actually attached to the map are hidden inside the component. It's much better to let the user have control over the shape array and copy it if they want to keep a backup before invoking the drawing manager

Ok, I think I can see the problem, I'm trying to avoid an antipattern, in Vue, it was possible to update props but from Vue 2 you shouldn't update propose from the child component you need to emit the new value and update it in the parent component you can check it here because of that I'm copying the array in the data object, but the shapes are not there the parent gets the updates from an event, in that way you could use a v-model too.

diegoazh commented 2 years ago
* On points 3 and 4 I'm not sure to understand well the case that you mention, for colours and other 
properties you can send it inside of the options property and it should work but maybe in these two last
questions should more easy to make a small repo to reproduce the issues and analyse the cases.

Have a look at code starting from this line

The setSelection and clearSelection functions use hard coded colours for the fill and border. What should really be happening is that we should have a prop to let the user specify border and fill colour for selected shapes, and we should return the shape back to either its original border and fill colour, or the border and fill colours specified in the shape options prop when it's deselected.

When I wrote this code, I made sure the hard coded values matched the options, and so the example code all looks fine. But if you change the border and fill colours in the sample code to, say, light red, then select and deselect a shape, you'll see it doesn't go back to red, it goes light grey because that's the hard coded colour in clearSelection

I agree with you we need to move this to props or use the options prop to set these values.

create-issue-branch[bot] commented 2 years ago

Branch issue-226-Bug_gmap-vue_2_0_latest_release_package_is_broken created!

davydnorris commented 2 years ago

Perhaps we should break these two issues out into their own separate ones and move the discussion there. I am happy to work on both. The selected colour prop will be pretty simple. The other will require more thought

diegoazh commented 2 years ago

@davydnorris I think we need to discuss these changes in another issue, can you open a new one because this issue was because of the release 2.0.0 doesn't work, I think in that way we organize the discussion and we can track in a better way the different problems

diegoazh commented 2 years ago

I propose the same as you before reading your comment 😅

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 2.0.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: