search

gabzim / circle-to-polygon

Receives a Coordinate, a Radius and a Number of edges and aproximates a circle by creating a polygon that fills its area
ISC License
113 stars 29 forks source link

Add TS typings #25

Closed zakjan closed 4 years ago

zakjan commented 4 years ago

@gabzim Hi, is this good to merge?

zakjan commented 4 years ago

Right, here you go

johachi commented 4 years ago

Perfect! Everything looks good. It's midnight and I need to sleep, but I will try it out tomorrow and merge it as soon as I have confirmed that it works as expected. :)

gabzim commented 4 years ago

One thing that has me doubting a bit is importing the typing from geojson. Mostly because it's not an official package and it's a utility to Turn your geo data into GeoJSON. Don't get me wrong, the typing is correct, the only thing I'm not certain about is having that project as a typing dependency. Anybody shares the same concern?

johachi commented 4 years ago

One thing that has me doubting a bit is importing the typing from geojson. Mostly because it's not an official package and it's a utility to Turn your geo data into GeoJSON. Don't get me wrong, the typing is correct, the only thing I'm not certain about is having that project as a typing dependency. Anybody shares the same concern?

You do have a point. I checked the code and everything looks good to me code-wise, but this was something I did not think about.

I'm honestly not very familiar with TS yet. @zakjan is there any benefit to have the dependency geojson compared to just declaring the types "manually"?

zakjan commented 4 years ago

Interesting. Sure the function type could be something like (center: [number, number], radius: number, numberOfSegments?: number) => { type: "Polygon", coordinates: [number, number][][] }, it probably compiles when using with other libraries together thanks to TS structural typing. It only lacks the proper type checking for library developers.

I see the benefit of using GeoJSON types in that it clearly declares the intent towards developers.

However, following https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html it would probably need @types/geojson in dependencies, not devDependencies, but that would install it also for non-TS downstream projects... Maybe for a non-TS library, the typings should better be only in definitelytyped.

zakjan commented 4 years ago

I have moved the typings to DefinitelyTyped, does it feel better? https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44372

gabzim commented 4 years ago

Excellent! Thanks @zakjan, that's very kind of you!

johachi commented 4 years ago

Thank you very much @zakjan !!

Also, if you want to remove the changes in this PR and instead update the readme.md with information about TS support, I promise to accept your PR within 1 day (I know I have been slow in regard to this PR, sorry about that).

zakjan commented 4 years ago

I still need to wait for the DefinitelyTyped PR to be merged. I'm not sure how long it takes for a new package. It was usually fast for updating existing packages.

Just don't merge this PR, it's unnecessary now :)