Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bounds are not of type LatLngBounds. #48

Closed
Saltibarsciai opened this issue Nov 5, 2020 · 26 comments
Closed

bounds are not of type LatLngBounds. #48

Saltibarsciai opened this issue Nov 5, 2020 · 26 comments

Comments

@Saltibarsciai
Copy link

Saltibarsciai commented Nov 5, 2020

fitBounds() does not work because bounds are not of type LatLngBounds.

Reproduce:

...
<l-map ref="map"></l-map>
....
...
this.map = this.$refs.map.leafletObject
this.map.fitBounds(map.getBounds());
...

This issue happens when accessing toLatLngBounds() through fitBounds() accessed from map ref

fitBounds(bounds) {
          blueprint.mapRef.fitBounds(bounds, {  
            animate: this.noBlockingAnimations ? false : null,
          });
        },

|
\/

fitBounds: function (bounds, options) {
	bounds = toLatLngBounds(bounds);
	if (!bounds.isValid()) {
		throw new Error('Bounds are not valid.');
	}
	var target = this._getBoundsCenterZoom(bounds, options);
	return this.setView(target.center, target.zoom, options);
}

|
\/

function toLatLngBounds(a, b) {
	console.log('b = ', b) //undefined
	console.log('a = ', a) //LatLngBounds
	console.log('a.constructor.name = ', a.constructor.name) //Object
	console.log('typeof a =', typeof a) // Object
	console.log('toLatLng instance = ', LatLngBounds) //function of LatLngBounds
	if (a instanceof LatLngBounds) {  // here bounds are of instance of Object but it should be LatLngBounds !!!!!!!!!!!!!!!!!!!!!!!!!!
		return a;
	}
	return new LatLngBounds(a, b);
}

But this issue is not happening when accessing toLatLngBounds() through imported 'leaflet' package

import L from 'leaflet'
...
app.use(L);
...
L.toLatLngBounds(bounds)  // here bounds are of instance LatLngBounds therefore it will return actual latlng

P.S.

Mainly tested on 0.1.2 because 0.2.0/0.3.0 has this issue #47 but also tried 0.3.0 (same)

@mikeu
Copy link
Contributor

mikeu commented Nov 5, 2020

Thanks for the detailed report, @Saltibarsciai ! Before I get too far into it, I just want to confirm something with you.

In your sample code, you have

let map = this.$refs.map
this.map.fitBounds(map.getBounds());

—should that either have this.map = ... on the first line, or simply map.fitBounds(...) on the second (no this)? Or how does this.map relate to the variable map?

@Saltibarsciai
Copy link
Author

@mikeu sorry, I tried to write it from my head as sketch, mine code is a bit more complicated, if you need reproduction repo I can create it.

this sketch is more correct, but still to actually reproduce you would need 2 nextTicks on 0.1.2
the point is that this line fails this.map.fitBounds(bounds) because Bounds type is object and not LatLngBounds

export default {
        name: "Test",
        data() {
            return {
                map: this.$refs.map
            }
        },
       mounted() {
            let bounds = L.latLngBounds(L.featureGroup(this.getSearchedItemMarkers()).getBounds());
            this.map.fitBounds(bounds, {maxZoom: 13});
       }

@Saltibarsciai
Copy link
Author

another thing:
let bounds = L.featureGroup(this.getSearchedItemMarkers()).getBounds();
typeof is LatLngBounds
and using it in this method
let bounds = L.latLngBounds(bounds);
returns itself (because it's typeof LatLngBounds)

now use the same bounds via map ref
this.map.fitBounds(bounds, {maxZoom: 13});
you'll get error that bounds are not defined, because it should do nothing, but it tried to create new bound from undefined second parameter

@mikeu
Copy link
Contributor

mikeu commented Nov 5, 2020

Thanks for the clarifications, @Saltibarsciai . This error reminds me very strongly of one I saw ages ago in Vue2Leaflet, long before I started working on its code, I've literally stepped through that same function, seeing Leaflet look at what should be a LatLngBounds object, then move on to create new bounds with an undefined second parameter :)

I think I've managed to reproduce the issue, but one more question to confirm—somewhere in there you end up calling this.$refs.map.leafletObject.fitBounds, right? Not just this.$refs.map.fitBounds?

@Saltibarsciai
Copy link
Author

@mikeu yes, good catch, I forgot to mention that!

@DonNicoJs
Copy link
Member

@Saltibarsciai Are you using webpack and vue-cli? Could you try adding an alias for leaflet on the vue-cli webpack config?

// vue.config.js
const path = require('path');
module.exports = {
  configureWebpack: config => {
      config.resolve.alias['leaflet'] = path.join(__dirname, 'node_modules/leaflet');
    },
  }
}

@Saltibarsciai
Copy link
Author

Saltibarsciai commented Nov 16, 2020

@DonNicoJs I'm using laravel mix. I added:

mix.webpackConfig({
        resolve: {
            alias: {
                '@': path.resolve('resources/'),// just to use relative path properly
                'vue': 'vue/dist/vue.esm-bundler.js',
                'leaflet': path.join(__dirname, 'node_modules/leaflet') // also tried path.resolve('node_modules/leaflet')
            }
        }
    })

but issue still occurs

@Saltibarsciai
Copy link
Author

reproduction demo: https://github.com/Saltibarsciai/leaflet-laravel

@DonNicoJs
Copy link
Member

@Saltibarsciai Thank you so much!

@DonNicoJs
Copy link
Member

@Saltibarsciai I've took a look at the reproduction demo and I've seen a 'pattern', can you try to remove

 "leaflet-gesture-handling": "^1.2.1",
 "leaflet.markercluster": "^1.4.1",

And see if ti works?

I am quite sure that this is the 'old' issue where leaflet is imported multiple times

@Saltibarsciai
Copy link
Author

Saltibarsciai commented Nov 25, 2020

@DonNicoJs Just did, pushed changes to repo, same error.
But shouldn't this

 'leaflet': path.join(__dirname, 'node_modules/leaflet')

in webpack fix multiple leaflets if it's true?
Screenshot_4

@Saltibarsciai
Copy link
Author

tested map bounds, they are valid for fitbounds

this.map.fitBounds(this.map.getBounds());

other bounds are not

this.map.fitBounds(L.latLngBounds(L.featureGroup(this.getSearchedItemMarkers()).getBounds()));

@Saltibarsciai
Copy link
Author

Saltibarsciai commented Nov 25, 2020

I temporary fixed this issue in production by renaming LatLngBounds to Object in node_modules/leaflet/dist/leaflet-src.esm.js on 1325th line, but this is temporary solution, hopefully we'll find a proper solution 👍

@DonNicoJs
Copy link
Member

this.map.fitBounds(L.latLngBounds(L.featureGroup(this.getSearchedItemMarkers()).getBounds()));

So this happens because leaflet inside compare with instanceOf I still think that somehow we are dealing with two different instances of leaflet I am digging on this right now!

@DonNicoJs
Copy link
Member

As I expected, if one tries:

<template>
  <div style="height: 75vh; width: 50vw;">
    <l-map ref="map" v-model:zoom="zoom" :center="center" @ready="readyHandler">
      <l-tile-layer
        url="https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png"
      ></l-tile-layer>

      <l-marker :lat-lng="[0, 0]" draggable @moveend="log('moveend')">
        <l-tooltip>
          lol
        </l-tooltip>
      </l-marker>
    </l-map>
    <button @click="changeIcon">New kitten icon</button>
  </div>
</template>
<script>
import "leaflet";
import { LMap, LTileLayer, LMarker, LTooltip } from "@vue-leaflet/vue-leaflet";
import "leaflet/dist/leaflet.css";

export default {
  components: {
    LMap,
    LTileLayer,
    LMarker,
    LTooltip,
  },
  data() {
    return {
      center: [47.41322, -1.219482],
      zoom: 2,
      iconWidth: 25,
      iconHeight: 40,
    };
  },

  methods: {
    log(a) {
      console.log(a);
    },
    async readyHandler() {
      const bounds = this.$refs.map.leafletObject.getBounds();
      const lBounds = window.L.latLngBounds(
        bounds._northEast,
        bounds._southWest
      );
      console.log(bounds instanceof window.L.LatLngBounds);
      console.log(lBounds instanceof window.L.LatLngBounds);
    },
  },
};
</script>

console.log(bounds instanceof window.L.LatLngBounds); will say true while
console.log(lBounds instanceof window.L.LatLngBounds); will say false

This is a tell that we have 2 different leaflet instances going on.

@DonNicoJs
Copy link
Member

@Saltibarsciai Okay I found a workaround for you! A decent one! Instead of doing

import L from 'leaflet'

If you use dynamic imports it works perfectly, here is how I modified the above function:

async readyHandler() {
      const { latLngBounds, LatLngBounds } = await import(
        "leaflet/dist/leaflet-src.esm"
      );

      const bounds = this.$refs.map.leafletObject.getBounds();
      const lBounds = latLngBounds(bounds._northEast, bounds._southWest);
      console.log(bounds instanceof LatLngBounds); // true
      console.log(lBounds instanceof LatLngBounds); // true
    },

@DonNicoJs
Copy link
Member

So what I am thinking is that when using dynamic imports and normal imports somehow we end up importing two different instances of leaflet

@mikeu This will pose a problem, I am thinking to add an utility function that will allow the user to specify what modules do they want from leaflet, it will import them and return them, otherwise this is going to become a nightmare

@DonNicoJs
Copy link
Member

DonNicoJs commented Nov 25, 2020

@Saltibarsciai @mikeu I finally found the culprit and the fix!

Inside vue-leaflet we are always importing from "leaflet/dist/leaflet-src.esm" to allow us to properly use the code splitting and tree-shaking feature!

@Saltibarsciai if you smply start importing directly from "leaflet/dist/leaflet-src.esm" and not from leaflet all your issues should go away!

You can check this codesanbox for how I came to the solution: https://codesandbox.io/s/awesome-hofstadter-48s8s?file=/src/index.js

We need to document this ASAP :) Or maybe even experiment if we can import directly from leaflet and still benefit from the tree-shaking 🤔

@Saltibarsciai
Copy link
Author

@DonNicoJs Awesome, you solved it!

import {latLngBounds, featureGroup} from 'leaflet/dist/leaflet-src.esm';

fixes the error!

This probably should be put in README.md as warning. Moving from vue2leaflet people will need to know. Maybe an upgrade guide should be created

@mikeu
Copy link
Contributor

mikeu commented Nov 26, 2020

Wow, thanks for tracking that down @DonNicoJs ! I agree it poses quite the potential issue going forward. I see a few possible approaches, including your initial suggestion, but no obvious winner yet... What are your thoughts on these and/or other options?

Utility dynamic importer

Give to the user some function that they pass names of Leaflet exports, and it returns the imported results. But using it will barely look different from just doing the import yourself anyway, I don't think, will it? If you have to say {a,b,c} = await vueLeafletImport(['a', 'b', 'c']) or similar, why not just say {a,b,c} = await import("leaflet/dist/leaflet-src.esm")?

Still, if you think there's a good approach that might simplify the whole process, or make it easier for us to enforce some kind of consistency, then I'd be happy to see it for sure!

Imports specified in props

A similar idea could possibly be accomplished through a prop on the LMap component, such as <l-map :load-from-leaflet="['latLngBounds', 'DomUtil.disableTextSelection']" ...>, then have them available either via inject or through a "Leaflet wrapper" function, after @ready.

Provide all of Leaflet by default

If there were some reliable approach to let users configure vue-leaflet without tying the library to any specific build system, we could simply import the whole of Leaflet by default and make it available to the user—e.g. provide('L', L) after importing it from the .esm file—but also have a mechanism to prevent this. Or possibly always use the same import, with a mechanism to specify a subset of Leaflet to import when not importing everything.

This could make it quite simple for new users or prototype apps to get access to all of Leaflet's functionality quickly, while providing a documented approach to allow tree shaking etc. later on. I don't have a great idea what the "reliable approach" or "mechanisms" that I mention would look like at the moment, though :).

Work around Leaflet's lack of module support

We aren't the only ones facing annoyances around the lack of support of modules. This issue thread contains a few possibilities, such as adding 'module' to the Leaflet package. I'm not sure how well it would work for a peer dependency, but maybe we could work out something along those lines.

If we could get this to work, I think it might be the only approach that wouldn't require fairly extensive documentation about how to work with both Leaflet and vue-leaflet side by side, since it would ideally allow us to switch all of our imports from the .esm file into more standard imports from leaflet.

All the documentation

Any solution that doesn't simply fix Leaflet so it works as expected is going to need a lot of documentation, I think. Not just details of how to import from Leaflet in a compatible manner, but quite prominent placement of the fact that you can't do it the standard way. Also, depending on the approach, there's the added fact that it might not be available until after onMounted. Also also, the effects on SSR and tree shaking of not following our approach, whatever that may end up being.

So I think most likely no matter which approach we go with, one of the above or something else, we'll also have to add this one. And it is not impossible that the best option will be to simply provide thorough documentation, with even our quickstart including it. The guide currently includes import L from "leaflet"; so that will have to change anyway. Maybe it will be sufficient to put a sample import there, with links to deeper in the docs about why it is that way.

@DonNicoJs
Copy link
Member

@mikeu I believe that for now, we best simply document this, I am afraid that anything we may end up implementing on our side will be rather hacky and not sturdy.

Maybe we should rather create a plugin-wrapper that can handle leaflet plugins that do not have support for vue-leaflet yet

@mikeu
Copy link
Contributor

mikeu commented Nov 26, 2020

@DonNicoJs I agree. We have a simple, effective solution that is easy enough to document and point people toward, so let's stick with that at the moment.

I like the idea of a wrapper at some point. Are you envisioning a vue-leaflet component that any user of the library could use, or a separate library that anyone could use to reasonably quickly publish a wrapped plugin?

@DonNicoJs
Copy link
Member

@mikeu I was thinking of a wrapper component yes :) something that could facilitate using custom code in the mounted hook or an event hook :)

In the meantime let's close this since we documented the fix!

@mathiash98
Copy link
Contributor

mathiash98 commented Feb 23, 2021

Met this issue myself Bounds are not valid, even though bounds.isValid(); from my own code returned true.

Can confirm that doing import * as L from 'leaflet/dist/leaflet-src.esm'; instead of import * as L from 'leaflet'; solves it.

@alexbainbridge
Copy link

Appreciate this is old but anyone struggling with this, a simple idea (and incomplete as doesn't solve the cause, just addresses the specific symptom) is to replace bounds with an array e.g. [bounds.getSouthWest(), bounds.getNorthEast()]

i.e.
this.$refs.map.mapObject.fitBounds(bounds, {padding: [40, 40], maxZoom: 14});
becomes
this.$refs.map.leafletObject.fitBounds([bounds.getSouthWest(), bounds.getNorthEast()], {padding: [40, 40], maxZoom: 14});

@HamzaAdam
Copy link

Is anyone has a solution for how to use [elmarquis] leaflet gesture handling library with vue-leaflet

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

No branches or pull requests

6 participants