Typescript implementation

[Falke-Design]

Hey,

there are many not implemented typescript declarations, so I made this issue to document the progress and the discuessions.

@ryan-morris @hoetmaaiers @lukekroon @elliots currently your are the hope of typescript for this library so it would be nice when you can work together and we can finally implemented a clean Typescript declaration.

[Falke-Design]

All issues / PR with TypeScript: #744, #678, #783, #747

[Falke-Design]

From @hoetmaaiers

I am currently updating my @ts-ignore parts to typings. I'll contribute these to the library once done. When I am unsure of the documentation I'll ask here.
Also the Typescript definition can be verified by @Falke-Design & @codeofsumit to ensure typings are correct?

[hoetmaaiers]

@Falke-Design, @codeofsumit a first update was provided. My experience form other libraries learns me that a d.ts file will never be completed and correct.

Is there any interest in refactoring leaflet-geoman to a typescript first approach? I would much more like to assist on this then porting my project specific types into this project (which in the end will pause and get out-dated with new versions etc.).

[Falke-Design]

what would it mean for us to refactor Geoman to typescript? We are no typescript developers, would it make problems for us to develop and understand the code?

[hoetmaaiers]

I won't romantisize the transfer form Javascript to Typescript. When starting some learning curve has to be taken.But the good thing, It is on top of javascript so everything you know remains, only clarity in types has to be added. The chances are hight you will even detect some bugs or uncaught scenario's.

Understanding the code will improve since types are like inline documentation.

since 3 years I write Typescript only, coming from 7 years of javascript development I can only encourage this.

[ryan-morris]

I agree with @hoetmaaiers points, the .d.ts files at best tend to lag behind new versions of projects. Some projects are not receptive to including them (see DefinitelyTyped) at all so I do appreciate your willingness to accept these in.

As far as refactoring to typescript, I would agree that I think the benefits would outweigh the learning curve. As the project gets bigger the safety and reassurances are well worth it and I wouldn't mind helping out on that endeavor. The leaflet types stay fairly up to date, and so any major leaflet changes would be easier to integrate or find potential issues.

I am still willing to help keep the d.ts up to date if that's not the direction you guys want to go. My initial PR just covered the surface area I was utilizing in our project, ideally if enough contributors come in that touch every part of this project, the coverage would be fairly complete and up to date.

[elliots]

It would be great to have it in typescript (both as a user, and someone who will hack on it a bit) but I currently have some trouble dealing with the innards of Leaflet from typescript. Only the public stuff is exposed, so when you need to get to private underscored methods, its a bit painful. Very likely I'm just doing it wrong though.

[Falke-Design]

Hey guys, can you please take a look on #820

[Falke-Design]

@ryan-morris or someone else, I think it would be really helpful if we had a demo for TyperScript. Do you think you can make this for us?

Demos:

• JavaScript: https://jsfiddle.net/o1dwu2vg/
• React: https://codesandbox.io/s/angry-babbage-cns52?file=/src/App.tsx
• Vue: https://github.com/mechevarria/demo-vue-leaflet-geoman/blob/master/src/components/Map.vue

[ryan-morris]

@Falke-Design I put together a very small demo here . It's an Angular project as that's what I'm most familiar with and could knock out the fastest. I'll try and take a look at the ones you linked and modify those to make use of the types but may take me a little bit to see how types work in React/Vue.

[ryan-morris]

Actually it looks like the React one you linked works fine if you update the @geoman-io/leaflet-geoman-free dependency on the left to 2.8.0 (the one that shipped the typings) or later, then they "just work". The problem is most of the references used have not been added to the type definition file here yet. If the typings were more complete, the only changes needed would be:

1. Change (mapElement as any).pm references to just mapElement.pm
2. Remove the PMDrawCircleEvent and PMEditCircleEvent types they included in favor of the ones shipped in typings from this project

[Falke-Design]

Thx for the demo. I want to add some demo links to the Readme. But my problem is, that I have no idea how Vue, React, Angular, ... works. I create a collection of links to demos but they are not from me. Do you know how React and Vue works? Then it would be nice to have up to date demos and have them more general (all buttons).

[ryan-morris]

That makes sense, wasn't sure if you just wanted a basic example of typescript integration or a more feature complete demo. I know I can expand on the angular version to be more like the included demo. Would you consider the included one the baseline you're trying to reach for the other demos? This would take a little more time as I'd like to/need to extend the included type definitions as I went through it.

As far as the other frameworks, I am familiar with how React works, but have not done any development with it. Vue has been on my radar for a while but have not looked at any implementation details at all. I wouldn't mind trying to put something together for them if no-one else is able. I have a pretty heavy workload the next few weeks but after that I should have some time and it would give me some practical experience with the others.

I think it would be worthwhile to add links to the ones you've found in the meantime. There's a lot of leaflet wrappers for these frameworks and they frequently have issues with how to add leaflet plugins on top of them so I think documentation here would actually be helpful to both this project and the wrapper communities such as react-leaflet and @asymmetrik/ngx-leaflet for angular.

The codesandbox/codepen/jsfiddle demos I think are more valuable as any issue filed could use those as a starting point to reproduce reported issues.

[codeofsumit]

Hi all, thanks a lot for the continued effort on helping us getting types ready for typescript users.
I wanted to chime in here regarding the discussion of switching the project completely to typescript.

While I understand the benefits of typescript in general, it's not a technology that myself or @Falke-Design are using anywhere in our projects or full-time jobs. You can argue that this shouldn't be the case, but it is. Moving this codebase to typescript is a huge undertaking for our onboarding into ts alone adding huge overhead to anything we do here (as we need to learn ts). Our time is already very constraint (I'm the bottleneck here) and adding anything that delays our development speed is not acceptable to me right now. I would need to learn ts solely for this library (and reviewing the PRs) and nothing else - it's not worth the effort to me.

However, I could see an acceptable scenario where the entire codebase moves to typescript if we have dedicated, commited engineers working on leaflet-geoman. So far, @Falke-Design defacto became the maintainer here and as long as that's the case, I wont advocate or push for any changes to our tech stack that would make his work (or mine) more difficult.

I hope that's understandable. Nonetheless, we are committed to help making this library ready for typescript users if we get the help from actual users like yourselves.

---

Regarding the demos:
@ryan-morris no requirement from our side regarding demo feature-completeness. A minimal viable example to show users how to integrate the library with their favorite framework should be sufficient.
A vue example should be able to be found using the github search by looking for leaflet-geoman in the package json and vue files in the project. Nonetheless, I could help with that as I use vue extensively.

Demo links via JSFiddle/Codesandbox or a similar tool for the different frameworks will be included in the README once we have them 👍👍

Thanks again for all your effort!

[Falke-Design]

I know this is very short term, but it would be cool if someone could take care of the PR #791. We probably want to do a release today (or tomorrow?) and it would be very nice if that were included.

Maybe @ryan-morris you have some time left

[ryan-morris]

@Falke-Design I didn't have anyway to modify the branch/PR in #791 so I forked off there and re-opened as #825 . I don't have time at the moment to cover the event related parts from the code review so I hope that works for now. I ended up jumping through 3 repos trying to keep @hoetmaaiers credited with original changes so let me know if anything looks amiss.

[hoetmaaiers]

Thank you @ryan-morris . Maybe we should gather Typescript related users to define a plan on how to make the library Typed without necessarily moving everything to Typescript.

[ryan-morris]

I did a quick pass through of the docs here to try and start getting everything covered. My biggest issue at the moment is the events conflicting with the leaflet types, @types/leaflet. The events there appear to be a little overreaching, I think it was to help enforce consistency on the leaflet side. My first pass is here if you have any thoughts

[Falke-Design]

@ryan-morris would it make sense to make a own project like @types/leaflet?
I don't asked @codeofsumit but I think it would be possible to make a new repo in the Geoman Project and give a few people control about it. Then you can make releases and changes without waiting on a release from our side.

[ryan-morris]

@Falke-Design that may not be a bad idea, especially in the short term while we know there are some issues / missing pieces. My hope is that everything will end up covered by the typings and remain fairly stable. I'm not sure at that point it would be ideal for end users since they'd need to keep 2 npm packages in sync to make sure they have the right typings package installed for the right geoman version.

[Falke-Design]

Ok yeah, the versions and the long support can be a problem. I think when we have the base, that I can add the (default) typings by my self for new PRs and then we have all new functions covered correctly.

FYI: We want to publish the next release in May if you want to add more typings.

[ryan-morris]

Ok good to know. I'll try to get all the event typings taken care of by then. That's the main blocker at this point from having everything in place.

[Falke-Design]

TypeScript wrong implemetation:

• TypeScript getGeomanDrawLayers improvement #865 getGeomanDrawLayers() Fixing PR: Fix #865 #868
• Imcomplete type definition #821 reInitLayers()

[ryan-morris]

@Falke-Design I have a little more testing I want to do before I open a PR, but if you get some time can you look over this draft . Mainly just to make sure the API surface looks correct. I went back through the README to try and make sure everything is covered.

There's a few things in there that are not ideal, including how the events are defined. It'll make them all show up on both Map and Layer as possible, but I haven't been able to work around that yet. I think until I get that it's better to have them showing up both places rather than not at all and that parameters/returns look to be correct. I went back through the current and past typescript issues to make sure I didn't undo anything but I may have overlooked something along the way.

The Utils aren't documented so I don't know if those are meant to be public. The pro ⭐ features I left off, since I'm not sure how the pro version is shipped/compiled and if it would just add confusion having them for the the -free version. Those can be added if needed.

[Falke-Design]

@ryan-morris Wow! Very strong, greate work!

I think the best would be to create a draft PR, then I can add a review.

To your points:

• Events: I also think that it would be better to have the events on both map and layer. In the next Release there will be also a change with the events. We moved all events to one place "EventMixin" Refactor events and create new Mixin-Class for events #836, you can look into it here maybe this helps you in some way. New is the source, also a custom payload will be added. The custom payload will be merged with the original payload and then passed to the fire function.
• Typescript Issues: I will check it too
• Utils: Yeah they are not documented but I want to add it in the readme. The following functions are public:
  • getTranslation(path) path = json string f.ex. tooltips.placeMarker
  • calcMiddleLatLng(map, latlng1, latlng2) returns the middle latlng between two points
  • findLayers(map) returns all layers that are available for Geoman
  • circleToPolygon(circle, sides = 60) converts a circle into a polygon with default 60 sides
• Pro Feature: Thanks for your offer, but we don't need them and I see it like you -> it confuse the users

Review
Events:

• pm:centerplaced will not longer pass marker
• pm:markerdragend intersectionReset is a boolean
• pm:intersect is missing
• pm:cut is missing
• pm:remove is defined twice (but is also available on layer and map)
• layerremove this is a Leaflet Event, so I think it is already defined in the Leaflet Types. Also we never fire this event directly so I would remove it from the list

PMLayer: Also some not documented functions on

• setOptions(options)
• getShape()
• enableLayerDrag() / disableLayerDrag() enables dragging for the layer
• dragging() returns if the layer is currently dragging

SUPPORTED_SHAPE_TYPES :

• ImageOverlay is missing (but only for Edit Mode).
• Are here the Leaflet types correct? L.Marker because we define them only as String this._shape = 'Line';

interface PMMap

• PMDrawMap twice: interface PMMap extends PMDrawMap, PMEditMap, PMDrawMap, PMRemoveMap, PMCutMap {
• You have added some function from L.PM.Map to PMDrawMap are you sure that this is good? This can confuse users because for example setGlobalOptions also set the options for existing layers (edit mode)
• getGeomanDrawLayers(): (asFeatureGroup: boolean) is missing

interface Draw:

• setPathOptions(options) is missing

EditModeOptions :

• hideMiddleMarkers boolean is missing

DrawModeOptions :

• finishOn: snap is missing
• layerGroup: move this to GlobalOptions, only there available

BlockPositions:

• Please also add options with a comment that this is pro version

ToolbarOptions :

• Please also add snappingOption, optionsControls, pinningOption with a comment that this is pro version

PMMapToolbar :

• I'm not sure if this line is correct: changeControlOrder(order: 'drawCircle' | 'drawRectangle' | 'removalMode' | 'editMode'[]): void; It can be each one of the "modes" : drawMarker, drawCircleMarker, drawPolyline, drawRectangle, drawPolygon, drawCircle, editMode, dragMode, cutPolygon, removalMode and every mode that is created with createCustomControl or copyDrawControl -> CustomControlOptions.name
• getControlOrder same as above
• setButtonDisabled(name: same modes as above, state: boolean) is missing

CustomControlOptions :

• block: is it possible to use BlockPositions? Else please add options ⭐

interface Button:

• tool is missing. Same as in CustomControlOptions block: BlockPositions

Action:

• onClick is optional

many, many thanks for your work, this is really a lot of work ❤

[robcdd]

I'd like to callout the matter that the types don't seem to cover how we can extract the LatLngs from geoman layers. Currently, the only way I see is

;(map.pm.getGeomanLayers() as Layer[]).forEach(l => {
  doStuff((l as any)._latlngs)
})

I'd like to encourage the authors to work through the pain of moving over to TS. It's not easy at first, but it will make your code stronger as others have alluded, potentially uncover bugs you didn't know about, and also increase library adoption.

I've cloned the project and I'll see if I can help at all. We'd like to use geoman where I work and it would help if the library had complete types.

[Falke-Design]

@robcdd
You can't call getLatLng() or getLatLngs() on a Layer-type. You need first cast to the right type.
Look: #1014

moving over to TS

We will not rewrite the code in TS. #790 (comment)

[robcdd]

@robcdd You can't call getLatLng() or getLatLngs() on a Layer-type. You need first cast to the right type. Look: #1014

This example works for event listener, but how do you get the geometries from a method call? Every property I see on the object returned from getGeomanLayers is marked as private.

[Falke-Design]

@ryan-morris can you help?

[ryan-morris]

A couple things:

1. @robcdd is this not what you are trying to accomplish?

(map.pm.getGeomanLayers() as L.Layer[]).forEach(layer => {
    if(layer instanceof L.Polygon) {
        const position = layer.getLatLngs();
    }
});

2. Some spots like this instance of getGeomanLayers may have been better mapped like:

getGeomanLayers(asFeatureGroup?: false): L.Layer[];
getGeomanLayers(asFeatureGroup: true): L.FeatureGroup;

instead of just

getGeomanDrawLayers(asFeatureGroup?: boolean): L.FeatureGroup | L.Layer[];

That would allow for slightly less typecasting. So for example, if we swapped the 1 getGeomanLayers declaration to the 2 I mentioned, I think what @robcdd is trying to accomplish would be more like:

// auto Layer[] detection
map.pm.getGeomanLayers().forEach(layer => {
    if(layer instanceof L.Polygon) {
        const position = layer.getLatLngs();
    }
});

// auto Layer[] detection
map.pm.getGeomanLayers(false).forEach(layer => {
    if(layer instanceof L.Polygon) {
        const position = layer.getLatLngs();
    }
});


// auto FeatureGroup detection
map.pm.getGeomanLayers(true).getLayers().forEach(layer => {
    if(layer instanceof L.Polygon) {
        const position = layer.getLatLngs();
    }
})

[raprocks]

is any work actively being done on this? would love to contribute if it is.

[Falke-Design]

@raprocks go for it. But please check out the out the other issues and PRs (#1111) too