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

Javascript errors with leaflet 1.1.0 #739

Open
4 tasks done
tomhughes opened this issue Jun 27, 2017 · 43 comments
Open
4 tasks done

Javascript errors with leaflet 1.1.0 #739

tomhughes opened this issue Jun 27, 2017 · 43 comments
Assignees
Labels

Comments

@tomhughes
Copy link

  • I'm reporting a bug, not asking for help
  • I've looked at the documentation to make sure the behaviour is documented and expected
  • I'm sure this is a Leaflet Draw code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
  • I've searched through the issues to make sure it's not yet reported

How to reproduce

  • Leaflet version I'm using: 1.1.0
  • Leaflet Draw version I'm using: 0.4.9

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
Copy link
Contributor

zakjan commented Jun 27, 2017

Another warning:

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

@jgravois
Copy link

@zakjan see #700 for more info.

@mvl22
Copy link

mvl22 commented Jun 27, 2017

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

@ewen
Copy link

ewen commented Jun 28, 2017

Also have the above problem.

@ayozebarrera
Copy link

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

@JonForest
Copy link

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
Copy link

frehner commented Jun 28, 2017

Leaflet/Leaflet#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
Copy link

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

@midnightmastermind
Copy link

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

@germanjoey
Copy link

germanjoey commented Jun 29, 2017 via email

@mjclawar
Copy link

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
Copy link

germanjoey commented Jun 30, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jun 30, 2017

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
Copy link

@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
Copy link

@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
Copy link

@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
Copy link

germanjoey commented Jun 30, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jul 1, 2017

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

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

@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
Copy link

germanjoey commented Jul 3, 2017 via email

@ddproxy
Copy link
Member

ddproxy commented Jul 3, 2017

@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
Copy link
Member

ddproxy commented Jul 3, 2017

@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
Copy link

@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
Copy link

mjclawar commented Jul 5, 2017

@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
Copy link
Member

ddproxy commented Jul 5, 2017

@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
Copy link

mjclawar commented Jul 5, 2017

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
Copy link
Member

ddproxy commented Jul 5, 2017

@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
Copy link

mjclawar commented Jul 6, 2017

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

@ddproxy
Copy link
Member

ddproxy commented Jul 6, 2017

@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.

@ddproxy ddproxy self-assigned this Jul 6, 2017
@ddproxy ddproxy added the bug label Jul 6, 2017
@Fillah
Copy link

Fillah commented Jul 19, 2017

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
Copy link

@Fillah Have you tried #739 (comment) ?

@vespaiach
Copy link

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
Copy link

m314 commented Jul 28, 2017

It looks like Leaflet devs implemented a fix for the non-extensible objects:
Leaflet/Leaflet#5650
Leaflet/Leaflet#5658

@sgentile
Copy link

sgentile commented Aug 6, 2017

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

@bcalik
Copy link

bcalik commented Aug 8, 2017

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

@scaddenp
Copy link

scaddenp commented Aug 9, 2017

Anyone tried it with 1.2.0?

@WorldMaker
Copy link

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
Copy link

glortho commented Aug 10, 2017

@scaddenp Confirmed 1.2.0 works

@brunob
Copy link
Contributor

brunob commented Jan 6, 2018

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

@mayacode
Copy link

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
Copy link

vinayakkulkarni commented May 14, 2018

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
Copy link

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
Copy link

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

@ryuchaehwa
Copy link

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

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

No branches or pull requests