luizbarboza / polyclip-ts

MIT License
26 stars 8 forks source link

Discussing compatibility with polygon-clipping types #7

Open markstos opened 10 months ago

markstos commented 10 months ago

I'm interested in having the Turf project replace polygon-clipping with tis library. ( related issue ).

I've started on the work, but I'm also less experienced working with types and have run into some cases I'm not sure how to resolved.

Geom public vs private

The polygon-clipping library has a Geom type. This ibrary has it too, but it does not seem to exposed. Here's how the type is used by turf-union:

const geoms: polygonClipping.Geom[] = [];

Can this module export the same type for compatibility?

number type vs Position type

turf-union has this code:

 const unioned = polyclip.union(geoms[0], ...geoms.slice(1));
  if (unioned.length === 0) return null;
  if (unioned.length === 1) return polygon(unioned[0], options.properties);

There, polygon method comes from @turf/helpers

If I swap in polyclip-ts, I get this typing error with npm run build:

 Argument of type '(number[][] | null)[][]' is not assignable to parameter of type 'Position[][][]'.

How can this typing difference be smoothed over?

Thanks!

dennemark commented 7 months ago

If this allows swapping polygon-clipping in turf.js, i also would appreciate that this issue could be resolved.

luizbarboza commented 7 months ago

@markstos Version 0.16.4 now exports the Geom type and should also resolve the typing error.

smallsaucepan commented 4 months ago

@markstos FYI now investigating this from the Turf side. Do you still have some bandwidth to be involved? https://github.com/orgs/Turfjs/projects/2

markstos commented 4 months ago

@smallsaucepan I still have some interest in helping with this, although I don't have access to view the link you sent: https://github.com/orgs/Turfjs/projects/2

It returns 404 for me, but I think that's often how Github handles access-denied cases.

It seems like if this project has resolved the typing issue, then I could pick up where I left off?

markstos commented 4 months ago

I see there was recent activity planning for Turfjs v7: https://github.com/Turfjs/turf/pull/2607

smallsaucepan commented 4 months ago

Thanks @markstos. Sorry about project visibility. It's basically pulling together some clipping related bugs - #2409, #2277, #2048, and #2588.

At this point waiting for Turf v7 to make it out the door. Then we can look at working on issues like this for 7.1, 7.2, etc

markstos commented 4 months ago

Should I start working on this as I have time or wait for the v7 release then?

markstos commented 3 months ago

7.0 is out now, so that question is answered. I'll proceed as time permits.

smallsaucepan commented 3 months ago

Sorry for not replying to your previous message @markstos. Yes, 7 is out now so let's carry on the discussion over at the Turf repo.