Leaflet / Leaflet.draw

Vector drawing and editing plugin for Leaflet
https://leaflet.github.io/Leaflet.draw/docs/leaflet-draw-latest.html
MIT License
1.98k stars 994 forks source link

Javascript errors with leaflet 1.1.0 #739

Open tomhughes opened 7 years ago

tomhughes commented 7 years ago

How to reproduce

What behaviour I'm expecting and which behaviour I'm seeing

I expect the javascript to load without throwing errors but it reports:

TypeError: can't define property "segmentsIntersect": Object is not extensible

Minimal example reproducing the issue

Load https://jsfiddle.net/1fuq748y/1/ while watching the console.

zakjan commented 7 years ago

Another warning:

Deprecated include of L.Mixin.Events: this property will be removed in future releases, please inherit from L.Evented instead.
jgravois commented 7 years ago

@zakjan see #700 for more info.

mvl22 commented 7 years ago

We can reproduce the above reports by @tomhughes and @zakjan in our own integrations.

ewen commented 7 years ago

Also have the above problem.

ayozebarrera commented 7 years ago

Same issue here.. "segmentsIntersect" error and mixin deprecation warning

JonForest commented 7 years ago

For a workaround on the "segmentsIntersect" bug you can pin the Leaflet version in your own package.json at "1.0.3". This will satisfy the semver requirements of the Leaflet-draw package.json and work. Not sure what changed in the Leaflet package between 1.0.3 and 1.1.0. Might go have a quick look and see, but from a quick glance earlier, it seems the answer was 'a lot'.

frehner commented 7 years ago

https://github.com/Leaflet/Leaflet/issues/5589

Does this appear to be the issue? If so, it looks like it's something that Leaflet-Draw itself would have to change.

JonForest commented 7 years ago

Yeah, that'd be it I reckon. Good find.

midnightmastermind commented 7 years ago

This is happening for me using Leaflet 1.0.3 and Leaflet-Draw 0.4.9

germanjoey commented 7 years ago

So what's the solution here? Just making L.LineUtil into its own class seems like it would be just fine? In fact, I don't see why it's extending L.Util to begin with...

On Wed, Jun 28, 2017 at 3:55 PM, Anthony Frehner notifications@github.com wrote:

Leaflet/Leaflet#5589 https://github.com/Leaflet/Leaflet/issues/5589

Does this appear to be the issue? If so, it looks like it's something that Leaflet-Draw itself would have to change.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/739#issuecomment-311814643, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNEqyZNZN464EqRStaA-deo0CyxYIsks5sItnJgaJpZM4OGj81 .

mjclawar commented 7 years ago

The update to Leaflet 1.1.0 also appears to break the test suite in Leaflet.draw because of the ES6+Rollup refactor. Should Leaflet.draw be updated for ES6+Rollup as well? Happy to contribute on that but I would assume that that change is relatively large in scope.

germanjoey commented 7 years ago

If you're volunteering to port over 5k lines of javascript and then reconfigure the build system - then by all means, please do.

On Thu, Jun 29, 2017 at 6:12 PM, Michael Clawar notifications@github.com wrote:

The update to Leaflet 1.1.0 also appears to break the test suite in Leaflet.draw because of the ES6+Rollup refactor. Should Leaflet.draw be updated for ES6+Rollup as well? Happy to contribute on that but I would assume that that change is relatively large in scope.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/739#issuecomment-312148340, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNEo-GIrihR9KNFpJXUE_4nQeyhFCJks5sJEuFgaJpZM4OGj81 .

ddproxy commented 7 years ago

Lots of good information here. It does appear the minor release of Leaflet broke Draw.

Any patches to resolve the utility issue should reference #5589 (thanks @frehner for pointing that out).

ES6 Rollup would be nice, we can set up an ES6 branch to handle that. Leaflet.Editable is having a similar issue (looks like utility) and the next 'major' update to Leaflet.Draw will be piggybacking off Editable - just an FYI.

In the meantime, I'll scope the compatibility of Leaflet.Draw to a ceiling of Leaflet 1.0.x. This will happen tomorrow morning.

JonForest commented 7 years ago

@midnightmastermind - was just thinking about why you'd still have the problem. Are you using npm3 or greater? Because npm3 stores all dependencies in a flat structure, so by adding Leaflet to your own package.json, it should not be refetched by Leaflet Draw. npm2 stores each dependencies' dependencies (?) in their own node_modules folder, so would be refetched. Hope that makes sense.

midnightmastermind commented 7 years ago

@JonForest thanks for the heads up. I actually found out the issue was my npm install was downloading the newest version due to the have "^1.0.3" as the dependency version. Removing the carot solved the issue.

mjclawar commented 7 years ago

@ddproxy I don't know if you had a timeline in mind for progressing on ES6+Rollup but I would be able to dig into that transition next week. I am indeed volunteering to do as much as needed to port over 5k lines of js and reconfigure the build system (and certainly willing to see it to the finish line since we use Leaflet.Draw pretty extensively on projects at StratoDem).

Do you want the ES6+Rollup to wait until Leaflet.Editable is updated, or can that progress in parallel?

germanjoey commented 7 years ago

@mjclawar so, that implies abandoning #651, among others? What advantages are you expecting from the ES6 port?

At any rate, it sounds like, at this point, if @ddproxy wants to take Leaflet Draw into another direction w/ Leaflet Editable, I should just permanently fork Leaflet Draw into another plugin entirely.

On Fri, Jun 30, 2017 at 2:38 PM, Michael Clawar notifications@github.com wrote:

@ddproxy https://github.com/ddproxy I don't know if you had a timeline in mind for progressing on ES6+Rollup but I would be able to dig into that transition next week. I am indeed volunteering to do as much as needed to port over 5k lines of js and reconfigure the build system (and certainly willing to see it to the finish line since we use Leaflet.Draw pretty extensively on projects at StratoDem).

Do you want the ES6+Rollup to wait until Leaflet.Editable is updated, or can that progress in parallel?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/739#issuecomment-312378639, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNElRCQMstBzsUbEwnUBxPNassGPzXks5sJWrhgaJpZM4OGj81 .

ddproxy commented 7 years ago

There are three different directions that can be taken at this point.

@mjclawar We can do the Rollup without Editable, since the Editable changes haven't made public yet (it's very alpha at this point) both can progress at their own rates.

@germanjoey I've been hesitant to approve PRs that make major changes or implement major features to this plugin. By using Editable, we can get access to their plugins that implement some of the features we are looking for. This should allow new features to remain optional, pluggable, and customizable that the current version of Draw struggles to maintain.

germanjoey commented 7 years ago

@ddproxy

I've gotta say, suddenly voicing this concern now after sitting on #651 for seven months (and despite my repeated attempts to follow up with you) is ridiculous. The concern about major changes also doesn't make sense, since I took great pains w/ L.Draw's #651 and L.Snap's https://github.com/makinacorpus/Leaflet.Snap/pull/40 to not make any major changes. The only modification to the existing logic was to flesh out the parameters for various L.Draw's events, for which I ensured backwards compatibility and also provided documentation. The other changes - restoring L.Snap compatibility, Undo/Redo, and providing L.CRS.Simple compatibility - were all completely optional/pluggable/customizable/whatever you wanna call it.

I don't really know what you're going for w.r.t. L.Editable integration with L.Draw. Feature-wise, as L.Editable seems to have every feature that L.Draw has except for the Toolbar?

And are you trying to say that you're done with this existing Leaflet Draw codebase?

Joe

On Sat, Jul 1, 2017 at 12:27 PM, Jon West notifications@github.com wrote:

There are three different directions that can be taken at this point.

  • ES6 Rollup
  • Bugfixes for L1.1
  • Editable integration

@mjclawar https://github.com/mjclawar We can do the Rollup without Editable, since the Editable changes haven't made public yet (it's very alpha at this point) both can progress at their own rates.

@germanjoey https://github.com/germanjoey I've been hesitant to approve PRs that make major changes or implement major features to this plugin. By using Editable, we can get access to their plugins that implement some of the features we are looking for. This should allow new features to remain optional, pluggable, and customizable that the current version of Draw struggles to maintain.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Leaflet/Leaflet.draw/issues/739#issuecomment-312451188, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzNErb4V-MSGttoT19nuYm0M9-P8mP_ks5sJp2JgaJpZM4OGj81 .

ddproxy commented 7 years ago

@germanjoey

No. Not done with the existing codebase.

651 introduces the undo and state managers that would qualify as a feature.

However, that is not why #651 has not been merged yet. The PR had a lot of merge conflicts and still does. Once these are resolved and I can test that PR, it may be merged.

ddproxy commented 7 years ago

@midnightmastermind 0.4.10 was released to handle the leaflet version cap.

@germanjoey I'm going to merge #651 into undo-manager this week. I'm okay with one more minor release (0.5.0) to handle pre Leaflet 1.1 to encompass those changes and ES6 can be 0.6.0. Once I see changes on #651 I'll merge and handle the remaining conflicts if any.

@mjclawar Looking at starting the ES6 rollup, due to bower, this will happen on a branch. If you have anything started, point me to it and I'll merge into es6-rollup

germanjoey commented 7 years ago

@ddproxy That would be great. I just pushed another commit onto #651 with some bugfixes and additional documentation. Please let me know if there's anything I can do to help.

mjclawar commented 7 years ago

@ddproxy I wasn't sure what direction this plugin was heading in given the above thread, so I ported Leaflet.draw to an ES6+Babel/Webpack project we use internally. We're probably going to split that out into its own public repo later this week, and I can point you at that when we do if that will be helpful. It has class implementations (e.g., class SimpleShape extends Feature), Flow type annotations and a fair number of ES6 syntax changes, so clearly it can't be merged easily into this plugin. Please let me know what would be most helpful for updates here.

ddproxy commented 7 years ago

@mjclawar That sounds like it's in the right direction. I favor Babel but since Leaflet uses Rollup, I'll cherrypick what is applicable. Thanks!

mjclawar commented 7 years ago

Here's where we're at right now: https://github.com/StratoDem/SDLeafletDraw. Obviously still needs a fair amount of work and I'll try to port the tests soon, but code is working for our use cases with an import 'sdleafletdraw;. This adds L.Control.Draw and L.Draw.Event access.

The ES6 code is in src/es6/, then built with Babel into dist.

ddproxy commented 7 years ago

@mjclawar Thanks for this. Looks like we stripped a lot of documentation annotations. I'm not sure how Rollup or Babel would treat those (probably ignore).

mjclawar commented 7 years ago

@ddproxy What do you mean by stripped a lot of documentation annotations? Where/when were they stripped?

ddproxy commented 7 years ago

@mjclawar There are annotations/code comments above each class and method in the original source code that are used to generate documentation of the api - that produces the api documentation.

Fillah commented 7 years ago

What is the fix for this bug? I'm working with a VueJS-PWA project where ES6 and Webpack is imported. When i'm trying to import leaflet and leafletDraw, it gives me the error:

Cannot add property segmentsIntersect, object is not extensible

I have imported it like this:

import Leaflet from 'leaflet';
import LeafletDraw from 'leaflet-draw';

The current versions are:

"leaflet": "^1.1.0",
"leaflet-draw": "^0.4.10"
JonForest commented 7 years ago

@Fillah Have you tried https://github.com/Leaflet/Leaflet.draw/issues/739#issuecomment-311659407 ?

vespaiach commented 7 years ago

from leaflet 1.1.x all Util objects are marked as non-extensible, that's why you got the error:

Cannot add property segmentsIntersect, object is not extensible

see this code in leaflet-src: var LineUtil = (Object.freeze || Object)

m314 commented 7 years ago

It looks like Leaflet devs implemented a fix for the non-extensible objects: https://github.com/Leaflet/Leaflet/issues/5650 https://github.com/Leaflet/Leaflet/pull/5658

sgentile commented 7 years ago

Is there going to be any attempt to update leaflet.draw to work with leaflet 1.1.0 . ? Thanks!

bcalik commented 7 years ago

Leaflet 1.2.0 is released. Probably solves the related issues. @ddproxy http://leafletjs.com/2017/08/08/leaflet-1.2.0.html

scaddenp commented 7 years ago

Anyone tried it with 1.2.0?

WorldMaker commented 7 years ago

Keep in mind those release notes point out that this is a temporary fix and eventually they will have new best practices not to add things to most L namespaces and instead start to use modules. I recommend cutting to disconnected modules sooner rather than later, for what little that advice is worth.

glortho commented 7 years ago

@scaddenp Confirmed 1.2.0 works

brunob commented 6 years ago

Since it works well with Leaflet 1.2.0, this issue may be closed (?)

mayacode commented 6 years ago

for me it doesn't work...

"leaflet": "1.2.0",
"leaflet-draw": "0.4.12",

and I get the warning: console.warn node_modules/leaflet/dist/leaflet-src.js:399 Deprecated include of L.Mixin.Events: this property will be removed in future releases, please inherit from L.Evented instead.

vinayakkulkarni commented 6 years ago

I got the Mixin issue:

leaflet-src.js?9eb7:400 Deprecated include of L.Mixin.Events: this property will be removed in future releases, please inherit from L.Evented instead. Error
    at checkDeprecatedMixinEvents (webpack-internal:///./node_modules/leaflet/dist/leaflet-src.js:402:47)
    at Function.Class.extend (webpack-internal:///./node_modules/leaflet/dist/leaflet-src.js:330:3)
    at eval (webpack-internal:///./node_modules/leaflet-draw/dist/leaflet.draw.js:8:1983)
    at eval (webpack-internal:///./node_modules/leaflet-draw/dist/leaflet.draw.js:9:26983)
    at Object../node_modules/leaflet-draw/dist/leaflet.draw.js (http://localhost/assets/js/app.js?id=d179acddb2f79c3f4ada:2377:1)
    at __webpack_require__ (http://localhost/assets/js/manifest.js?id=360cf4a2ac3af2c3da19:55:30)
    at eval (webpack-internal:///./resources/assets/js/app.js:22:72)
    at Object../resources/assets/js/app.js (http://localhost/assets/js/app.js?id=d179acddb2f79c3f4ada:3712:1)
    at __webpack_require__ (http://localhost/assets/js/manifest.js?id=360cf4a2ac3af2c3da19:55:30)
    at Object.0 (http://localhost/assets/js/app.js?id=d179acddb2f79c3f4ada:3875:1)

package.json has:

"leaflet": "^1.3.1",
"leaflet-draw": "0.4.2",
danielnaab commented 6 years ago

What's the status on a ES6/Rollup build? I see there's a 2.0.0-fork branch that looks like an attempt at this. Is there a separate issue to track that, and is there somewhere specific where help could be offered?

ryuchaehwa commented 4 years ago

Hello All, I'm still having this issue. Any updates yet??

ryuchaehwa commented 4 years ago

Working Example:

Hey All, Just to let someone know if anyone still having this issue. I'm useing leaflet(1.6.0) vuejs, vue2-leaflet as wrapper and leaflet-draw(0.4.X), edit, drag, handler to customize functions and buttons.

I couldn't figure it out how to fix it so I've made a component by adding leaflet-draw, edit, drag, handler valina js(and customized a bit) altogether in one component.. I don't use npm for this plugin functions anymore and now it works perfect. So it's better to convert vanilla code to vuejs component and handle all the code there even it's over 8,000 lines....... hehe :)

Anyway, let me know if you need more info.

BTW, you need to fix the vanilla code a bit as all the variables are using only 'var'. It will cause some problems to sending data to leaflat(1.6.0, npm installed) as leaflat, leaflet-draw, edit, drag, handlers are using same variable names and you need to put them in one component. I had an issue that leaflet(npm) couldn't recognize which event was occurred(error methods in leaflet.js = .trim()).

component should be like this

//leaflet-draw template in vuejs
<template>
<div style="display: none;"> </div>
</template>

<script>
add all the vanilla js here and fix the codes as you need

let map = {}
let whatever layer, group you need = L.featrureGroup();

export default {
 name: 'leaflet-draw',
mounted() {
 this.$nextTick(() => {
 //write here to mount buttons and functions
// e.g.
//map = this.$parent.$parent.$refs.map.mapObject
)
}
}
</script>
parent component where map is(i'm using vue2-leaflet wrapper)
<template>
 <l-map>
   <l-tile-layer />
  <leaflet-draw position="topleft" />
</l-map>
</template>

<script>
import LeafletDraw from './LeafletDraw';

 export default {
..
components: {
'leaflet-draw': LeafletDraw
}
</script>

and that's all. Here's an output(I'm still working on button icon as we're using fontawesome... please ignore it)

image