search

geoman-io / leaflet-geoman

🍂🗺️ The most powerful leaflet plugin for drawing and editing geometry layers
https://geoman.io
MIT License
2.18k stars 431 forks source link

Typescript implementation #790

Open Falke-Design opened 3 years ago

Falke-Design commented 3 years ago

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 commented 3 years ago

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

Falke-Design commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

Falke-Design commented 3 years ago

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

ryan-morris commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

TypeScript wrong implemetation:

ryan-morris commented 3 years ago

@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 commented 3 years ago

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

Review Events:

PMLayer: Also some not documented functions on

SUPPORTED_SHAPE_TYPES:

interface PMMap

interface Draw:

EditModeOptions:

DrawModeOptions:

BlockPositions:

ToolbarOptions:

PMMapToolbar:

CustomControlOptions:

interface Button:

Action:

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

robcdd commented 2 years ago

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 commented 2 years ago

@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. https://github.com/geoman-io/leaflet-geoman/issues/790#issuecomment-804270280

robcdd commented 2 years ago

@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 commented 2 years ago

@ryan-morris can you help?

ryan-morris commented 2 years ago

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();
    }
});
  1. 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();
    }
})