Skip to content

Increase strictness of @turf/line-chunk input #2970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mfedderly wants to merge 1 commit into
base: master
Choose a base branch
from
+70 −8
Open

Increase strictness of @turf/line-chunk input #2970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions packages/turf-helpers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,15 +598,20 @@ export function multiPolygon<P extends GeoJsonProperties = GeoJsonProperties>(
* // => collection
*/
export function geometryCollection<
T extends
| Point
| LineString
| Polygon
| MultiPoint
| MultiLineString
| MultiPolygon,
P extends GeoJsonProperties = GeoJsonProperties,
>(
geometries: Array<
Point | LineString | Polygon | MultiPoint | MultiLineString | MultiPolygon
>,
geometries: Array<T>,
properties?: P,
options: { bbox?: BBox; id?: Id } = {}
): Feature<GeometryCollection, P> {
const geom: GeometryCollection = {
): Feature<GeometryCollection<T>, P> {
const geom: GeometryCollection<T> = {
Copy link
Collaborator Author

@mfedderly mfedderly Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has an interesting backstory.

packages/turf-line-chunk/types.ts has lineChunk(geomCollection, 2);, but geometryCollection was erasing the specific subtype (the default is just GeometryCollection<Geometry>) instead of inferring them forward into the return type. This changes geometryCollection to preserve any sort of information we did have for the things getting put into the collection.

I believe that this class of type strictness error may hit other consumers if they are creating geometry collections using other methods.

type: "GeometryCollection",
geometries,
};
Expand Down
12 changes: 9 additions & 3 deletions packages/turf-line-chunk/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
* If the line is shorter than the segment length then the original line is returned.
*
* @function
* @param {FeatureCollection|Geometry|Feature<LineString|MultiLineString>} geojson the lines to split
* @param {FeatureCollection|Geometry|Feature<LineString|MultiLineString>} geojson the LineString or MultiLineStrings to split
* @param {number} segmentLength how long to make each segment
* @param {Object} [options={}] Optional parameters
* @param {Units} [options.units='kilometers'] Supports all valid Turf {@link https://turfjs.org/docs/api/types/Units Units}
Expand All @@ -34,8 +34,8 @@ function lineChunk<T extends LineString | MultiLineString>(
| Feature<T>
| FeatureCollection<T>
| T
| GeometryCollection
| Feature<GeometryCollection>,
| GeometryCollection<T>
| Feature<GeometryCollection<T>>,
segmentLength: number,
options: {
units?: Units;
Expand All @@ -57,6 +57,12 @@ function lineChunk<T extends LineString | MultiLineString>(

// Flatten each feature to simple LineString
flattenEach(geojson, (feature: Feature<T>) => {
if (feature.geometry.type !== "LineString") {
throw new Error(
"Only LineString and MultiLineString geometry types are supported"
);
}
Copy link
Collaborator Author

@mfedderly mfedderly Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to handling points differently here, but this has the nice property of aligning the TypeScript strictness with the runtime behavior.

Copy link
Member

@smallsaucepan smallsaucepan Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, what do we want Turf's runtime behaviour to be when it's gets unexpected / inappropriate input? Halt on unexpected input? Or soldier on silently (or with a warning)?

Copy link
Collaborator Author

@mfedderly mfedderly Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think about this along a few axes.

The first is something like "what is this method supposed to accept?", in this case we have a method that only makes sense on line-shaped things, so points and polygons should be considered invalid and strongly rejected. But we should take a more moderate stance on the container shape. Send a LineString directly, or a FeatureCollection<Feature<GeometryCollection<MultiLineString>>> if you want, and we'll try to handle it.

The second is the type checking errors vs runtime errors. Ideally these would be consistent, but I think its safer to add strictness at type checking time (it won't surprise the consumer in production). We're flagging things that are likely already buggy at runtime for them anyhow. At one point we started removing the runtime checks because we added guards at the type level, but I'm not sure we'll ever be able to get away with that because the runtime guards provide way clearer feedback to the dev than some error deep within the implementation.

I'm not a big fan of console.warn or similar from within a library, large apps may have their own logging systems which that make this form of feedback pretty difficult to manage (do you shim out console before calling some arbitrary function just to grab the message and repeat it into your own logging?). It also seems less likely to get noticed and fixed than just being loud and tossing an Error.

Overall I think the current state of this PR reflects my idea of what 'ideal' looks like in this case, but I understand that we might be reluctant to actually merge this without a major release because of the risk of runtime breakage. I would certainly consider removing the runtime error for Point-likes, and changing that to a warn or keeping it silent for now, and coming back with yet another PR when we do a major break. You could even make the case that the potential TypeScript errors from making the GeometryCollections more strict would itself require a major bump, and therefore we can shelve this entire PR for the 8.x milestone and revisit it later.

We have a pretty limited velocity on this project, and blocking little improvements like these until the next major winds up delaying the improvements getting rolled out to users. I'm not sure what the right balance is between getting things out quickly vs disrupting the folks that are tracking our releases more closely.

Copy link
Member

@smallsaucepan smallsaucepan Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

points and polygons should be considered invalid and strongly rejected. But we should take a more moderate stance on the container shape

This seems a good balance. Caller has to be more alert about what they pass in. So for a geometry collection containing 99 lines and 1 point, we would throw?

coming back with yet another PR when we do a major break

As is I think this would have to be a major release candidate:

  • now throws where we previously silently ignored invalid geometries
  • narrower input types could break builds

To keep moving forward between majors then, do we make this PR about the helpers.geometryCollection part? That doesn't narrow input types, and doesn't broaden the return type either. Runtime behaviour would stay the same.

Copy link
Collaborator Author

@mfedderly mfedderly Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll send this to the v8 milestone and go look at everything in helpers tomorrow to see if there's anything else that can get a similar treatment.

Copy link
Collaborator Author

@mfedderly mfedderly Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got eager and broke that out here #2971


// reverses coordinates to start the first chunked segment at the end
if (reverse) {
feature.geometry.coordinates = feature.geometry.coordinates.reverse();
Expand Down
51 changes: 51 additions & 0 deletions packages/turf-line-chunk/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,57 @@ test("turf-line-chunk: Prevent input mutation", (t) => {
t.end();
});

test("turf-line-chunk: Validate behavior on unsupported geometry types", (t) => {
// Point | MultiPoint | LineString | MultiLineString | Polygon | MultiPolygon | GeometryCollection;

// each of these `as any` casts are required to bypass the TypeScript rejection and actually test the validation underneath
t.throws(() => {
lineChunk({ type: "Point", geometry: [0, 0] } as any, 10);
}, "Point geometries are rejected");

t.throws(() => {
lineChunk({ type: "MultiPoint", geometry: [[0, 0]] } as any, 10);
}, "MultiPoint geometries are rejected");

t.throws(() => {
lineChunk(
{
type: "Polygon",
geometry: [
[
[0, 0],
[1, 0],
[1, 1],
[0, 0],
],
],
} as any,
10
);
}, "Polygon geometries are rejected");

t.throws(() => {
lineChunk(
{
type: "MultiPolygon",
geometry: [
[
[
[0, 0],
[1, 0],
[1, 1],
[0, 0],
],
],
],
} as any,
10
);
}, "MultiPolygons geometries are rejected");

t.end();
});

/**
* Colorize FeatureCollection
*
Expand Down