Typescript-ifying modules

[smallsaucepan]

Converted so far:

turf-buffer
turf-difference
turf-dissolve
turf-ellipse
turf-envelope
turf-explode
turf-flatten
turf-flip
turf-geojson-rbush
turf-great-circle
turf-interpolate
turf-line-chunk
turf-line-offset
turf-line-slice-along
turf-line-slice
turf-line-split
turf-mask
turf-meta
turf-midpoint
turf-planepoint
turf-point-on-feature
turf-points-within-polygon
turf-polygon-smooth
turf-polygon-tangents
turf-rewind
turf-sample
turf-sector
turf-shortest-path
turf-simplify
turf-square
turf-standard-deviational-ellipse
turf-tag
turf-tesselate
turf-transform-rotate
turf-transform-scale
turf-transform-translate
turf-unkink-polygon
turf-voronoi

Currently converting a few modules to Typescript. Some have been pretty straightforward (e.g. turf-flip) while others are looking a little more gnarly. Starting this discussion as a means to get some second opinions.

turf-meta

findPoint

findPoint is defined in index.d.ts as returning Feature<Point, P> though it pretty clearly also returns null. Options:

1. change definition to return Feature<Point, P> | null
2. change definition to return Feature<Point, P> | undefined and return undefined if the element requested doesn't exist
3. something else

Option 2 seems best to me. I feel undefined implies non-existence more clearly than null. findSegment is similar.

coordEach

coordEach returns void as does the callback passed by the user. However, in many places we see this pattern:

if (
  callback(
    coords,
    coordIndex,
    featureIndex,
    multiFeatureIndex,
    geometryIndex
  ) === false
)
  return false;

Typescript is stricter and rightly complains when we try to return boolean from a void function. Also, this would abort the traversal of all the elements as soon as the callback ever returned false. No mention of this in the docs so it would be very unexpected.

My gut feel is we strip out the logic and completely discard the result as the interface suggests. Any concerns with that? If anyone is checking the result of coordEach they're relying on something undocumented. Same would go for propEach, featuerEach, etc

Thoughts on this? Thanks

@mfedderly @JamesLMilner @twelch

[twelch]

Thanks @smallsaucepan

findPoint - I think returning undefined if nothing to return sounds goods. I wonder if there's a larger precedence of returning null?

Have you seen this WIP PR #2275? I wonder if there's some relevant overlap, at least in usage of the *Each functions. I also wonder if it's worth landing this first, it was getting pretty close to ready.

coordEach - I realize the behavior of returning a boolean is undocumented, but I believe I stumbled on it as well at some point and have utilized it as a way to make the function bail out, rather than proceed to iterate through all elements. I think that has value. Thoughts on how we can keep or still achieve that behavior? And I wonder if the *Reduce functions or other internal users of *Each functions utilize it.

[smallsaucepan]

Thanks Tim. Had seen #2275. Wasn't sure its status though, noticing a few places where it seemed inconsistent. For example coordEach now returns false | void but propEach and featureEach were still only void.

Obviously though a work in progress - so perhaps this discussion can help us establish what we want to do, and then we can figure the best avenue to get there.

I have the opposite opinion regarding early bail out, especially as the function mutates in place. Now I've potentially got a half processed piece of geojson with no idea at which point it stopped. Do you recall the use cases where that behaviour was an advantage? It might make more sense to keep Each functions predictable and add an additional category i.e. findFirst or coordEachUntil.

The Reduce functions do use the Each functions, but define their own callback which doesn't interpret the result and isn't a way to abort the reduction.

[twelch]

I can't find it, but the use case I have in mind is for optimization. Say I have a big complex array, and I want to fail as soon as possible, if I've found a condition that is out of spec. You're right that the return value of a boolean from *Each() doesn't tell you much. What I'm really wanting is just the break behavior. So perhaps return false could just become break?

if (
  callback(
    coords,
    coordIndex,
    featureIndex,
    multiFeatureIndex,
    geometryIndex
  ) === false
)
  break;

Then I could do the following, capturing the why it broke, in variables defined higher in the stack.:

let isError = false
let errorGeometry: Geometry
let errorFeatureIndex: number | undefined
geomEach(complexFeatures, async (curGeometry, featureIndex) => {
  if (meetsSpec(curGeometry) === false) {
    isError = true
    errorGeometry = curGeometry
    errorFeatureIndex = featureIndex
    return false  // will trigger break
  } else {
    do something else
  }
});
if (isError) {
  handleError(errorGeometry, errorFeatureIndex)
}

There's another part of geomEach that caught my eye again, which is handling of null geometry:

      if (geometry === null) {
        if (
          callback(
            null,
            featureIndex,
            featureProperties,
            featureBBox,
            featureId
          ) === false
        )
          return false;
        continue;
      }

If I'm reading this correctly, if geometry is null, then it will callback() passing null. I think this is designed to give the caller control over what happens if the geometry is null, and let the caller return true if they want it to continue or false in order to make it break. This seems to still work if return false is switched to break as suggested above.

From a higher level what do we want perspective? I can see giving the caller the ability continue or skip just like a regular for loop as much as possible, including control over when it happens.

[smallsaucepan]

To summarise then *Each continue to return void, but the callback has the signature void | boolean and false directs *Each to complete early. It's left to the caller to wrap enough state with the callback to know if and why that happened. Should be able to make that consistent.

[twelch]

Yes, that sounds right.

[smallsaucepan]

Here's a first go at typing one of the reducers. Mind taking a look and letting me know if you can see any obstacles?

This should cater for the situation where the type of return value we expect from reduce is totally different from the type of element we're processing e.g.

// callbackfn simply appends all the coords together as strings.
const longUselessString = coordReduce<string>(geojson, callbackfn, "");

Not sure what happens in the above case though when we don't pass an initial value ...

export function coordReduce<U>(
  geojson: AllGeoJSON,
  callback: (
    accumulator: U,
    currentCoord: Coord,
    coordIndex: number,
    featureIndex: number,
    multiFeatureIndex: number,
    geometryIndex: number
  ) => U,
  initialValue?: U,
  excludeWrapCoord?: boolean
): U {
  let accumulator = initialValue;
  coordEach(
    geojson,
    function (
      currentCoord: Coord,
      coordIndex: number,
      featureIndex: number,
      multiFeatureIndex: number,
      geometryIndex: number
    ): void {
      if (coordIndex === 0 && initialValue === undefined) {
        // Initialise accumulator to "array[0]" and skip to second iteration.
        accumulator = currentCoord as U;
      } else {
        // Cast accumulator to U as Typescript believes it could be
        // U | undefined
        // The if above should stop that from ever happening. A side effect
        // of the way we're implementing reduce() with *Each.
        accumulator = callback(
          accumulator as U,
          currentCoord,
          coordIndex,
          featureIndex,
          multiFeatureIndex,
          geometryIndex
        );
      }
    },
    excludeWrapCoord
  );

  // Same cast as above, as Typescript can't tell we'll have assigned
  // "array[0]" to accumulator if initialValue was not provided.
  return accumulator as U;
}

[twelch]

The only potential obstacle off the top of my head is that I remember an optional default value might have played into creating a return value of undefined on migrating I think geomReduce in #2275 - #2275 (comment)

[smallsaucepan]

Is there a reason all the callback args for segmentReduce and lineReduce are optional?

  callback: (
    previousValue?: Reducer,
    currentSegment?: Feature<LineString, P>,
    featureIndex?: number,
    multiFeatureIndex?: number,
    segmentIndex?: number,
    geometryIndex?: number
  )

[twelch]

Not that I know of. I've studied the reduce functions less. If they're always passed then no reason they should be optional 🤷

[mfedderly]

I think that they might not be set if you get callbacks as it descends through a containing larger block of geojson? Its been ages there's more cobwebs than memories for me.

[mfedderly]

Sorry just getting caught up.

I think that we have code that uses null because undefined was not always a reserved keyword.

You have already linked to #2275 but I've lost track of its current status. It must not be finished if there's inconsistencies that you spotted already. I think you're right though that returning false was intended to short-circuit the iteration and is probably important for performance in some places. @turf/meta isn't really intended to be used outside of our own packages IMO so I'm not super worried about changing behavior being a large burden to consumers. Having strongly typed @turf/helpers and @turf/meta seems pretty important to making sure that the rest of the Typescript packages can be written with good underlying safety.

With a wider view, we still have a bunch of Javascript-only packages that could maybe be converted to get rid of the weird split build system. I didn't try to undertake the conversion a while ago because @rowanwins felt strongly that Typescript wasn't popular enough at the time and it may discourage contributions from the community. With the benefit of a little more hindsight in 2023 I wonder if we might be more comfortable moving entirely to Typescript now.

[twelch]

I find turf/meta functions very helpful as a heavy user of turf and consumer/producer of geojson. It lets me more easily build larger more complex spatial functions, that work with various types of spatial input. I think it's good that we expose them publicly.

Granted I don't use them all but the *Each and *Reduce are probably the most useful.

[mfedderly]

I find turf/meta functions very helpful

Perhaps this is a justification for #2537 that we should actually expose the inner shared code and just treat it as public. It also makes me less supportive of making big changes to the public APIs in @turf/meta assuming that the monorepo was the only consumer.

[smallsaucepan]

Some low level shared code should remain hidden a la #2537. Limiting that to non-public, currently duplicated nuts and bolts code seems a good yardstick.

On the turf-meta front though we can keep them public. I'm aiming to keep the interface as close to the current as possible for all these packages. There will probably be a couple of areas where it being JS meant it could be used more broadly than the d.ts suggested. So they might be more limited, though in most cases we're passing geojson and switching based on what we get - not something we're enforcing at the TS level.

Maybe that's the prompt to (sooner rather than later) look more in to overloading. Maybe we can leverage TS types more that way and cut back on some switch statements.

Also suspect we can cut back on generic parameters. For example, in a lot of cases I'm not sure why we parameterise GeojsonProperties. It only seems to be to pass it into another turf function we've also defined a generic prop on.

[twelch]

The way Ive found the geojson properties generic parameter to be useful is to pass in more strongly typed feature properties object than the default geojson types define (which is nothing), and the return will keep that strong type. It avoids extra checks or casting in user space to maintain it.

Seems like an admirable effort for functions where it can be guaranteed 🤷

[smallsaucepan]

Thanks Tim. That's a good use case to have.

I have come across another related issue to do with types, this time on the geojson objects.

For example, in turf-points-within-polygon we can pass a geometry F that extends Point or Multipoint, and the function then uses inference and generics to say it will return a FeatureCollection of F. So pass in a Point get Points out, pass in a MultiPoint get MultiPoints out. Good so far.

Theoretically though the caller could pass something that actually extends MultiPoint (e.g. a SuperMultiPoint with additional required fields). pointsWithinPolygon() uses turf-helpers multiPoint() which only knows how to create vanilla MultiPoints. So were I to pass in an arg of type SuperMultiPoint, F is now SuperMultiPoint and there's no way multiPoint() can know what extra fields it has to create to satisfy that type shape.

Typescript complains with an error along these lines:

'MultiPoint' is assignable to the constraint of type 'F', but 'F' could be instantiated with a different subtype of constraint 'Point | MultiPoint'

We could get around this by casting the vanilla MultiPoint to Feature<F, P> as soon as we get it back from multiPoint(), but that's badly broken. What the user (and their tools) think is a returned SuperMultiPoint doesn't actually have any of the additional fields. Referencing them results in a runtime error.

The responsible way to go seems to be to overload the function - one variant focusing on Point and the other MultiPoint:

// When passed a Point (or subtype)
function pointsWithinPolygon<
  G extends Polygon | MultiPolygon,
  P extends GeoJsonProperties,
>(
  points: Feature<Point, P> | FeatureCollection<Point, P>,
  polygons: Feature<G> | FeatureCollection<G> | G
): FeatureCollection<Point, P>;

// When passed a MultiPoint (or subtype)
// SuperMultiPoint in, **MultiPoint** out
function pointsWithinPolygon<
  G extends Polygon | MultiPolygon,
  P extends GeoJsonProperties,
>(
  points: Feature<MultiPoint, P> | FeatureCollection<MultiPoint, P>,
  polygons: Feature<G> | FeatureCollection<G> | G
): FeatureCollection<MultiPoint, P>;

Essentially, any of our functions that create points, features, etc can't return anything more than the standard geojson types. At first glance that seems to be the gist.

Thoughts? Any alternative approaches you can think of?

[mfedderly]

Super good catch with the generics

I think you can just infer it on the return type? Maybe pick a better variable name than X (its early, I'm still waking up!)

function pointsWithinPolygon<
  G extends Polygon | MultiPolygon,
  P extends GeoJsonProperties,
  X extends Point | MultiPoint
>(
  points: Feature<X, P> | FeatureCollection<X, P>,
  polygons: Feature<G> | FeatureCollection<G> | G
): FeatureCollection<X extends Point ? Point : MultiPoint, P>;

[smallsaucepan]

After some thoughts on turf-shortest-path @DenisCarriere @mfedderly

Docs say default resolution is 100. Looking at the code, resolution is actually calculated as the width of the bbox divided by 100. And then used to calculate the cell width and height.

turf/packages/turf-shortest-path/index.js

Lines 83 to 86 in c8b37e8

	if (!resolution) {
	var width = distance([box[0], box[1]], [box[2], box[1]], options);
	resolution = width / 100;
	}

This works in the tests because resolution isn't passed, so the if above is executed and resolution is overwritten with 6.23979 (or whatever). If I explicitly pass 100 as an option the code bombs because resolution stays as 100. I think we just need to replace the hard coded 100 with whatever is passed in as resolution.

Question then, should the option name change - divisions maybe? - and the meaning of the resolution variable stay the same in the code. Or vice versa?

Also if someone passes the minDistance option the code throws with "not implemented". It's been that way for 6 years. Should we just remove minDistance altogether? Found #2291 so maybe we thought it had already been done.

[mfedderly]

If we made it clear that resolution is in degrees of longitude maybe it would at least be clearer? I'm not really sure how useful it is in its current form though from a consumer side. I will admit that I haven't spent much time looking at turf-shortest-path though.

In favor of removing minDistance until someone decides to implement it.

Its probably worth calling out that even if you make the types more strict, the js-time typechecking is part of our API and people may complain if it gets removed.

[smallsaucepan]

If we made it clear that resolution is in degrees of longitude maybe it would at least be clearer?

Ah ok. I'll rejig the documentation. The code is still wrong though, right? Or does the hard coded 100 have some significance I can't see?

Yeah, I think the "line of sight / shortest path" technique I saw mentioned elsewhere is interesting. This is the current path astar spits out for one of our test cases, so there are definitely better options:

I'll drop any mention of minDistance from the API for now. And no plan to remove any of the runtime checks beyond this unused param.

[mfedderly]

Yeah I'm not sure why it has the hardcoded 100 🤷 . Its hard for it to be "wrong" when there isn't really strong documentation about how its supposed to behave I guess.

[smallsaucepan]

So the resolution is basically the cell width in whatever units are passed (default 100km?) and we divide a bounding box up into however many cells of that size. How about this for the documentation:

{string} [options.units='kilometers'] unit in which resolution & minimum distance will be expressed; it can be degrees, radians, miles, kilometers, ...
{number} [options.resolution=100] distance between matrix grid points through which the path will be calculated. Smaller values lead to more precise paths

Not sure if that last statement about precision is true though. Guessing that's the point of the parameter?