From 7275eb900f1ac7e1499e59ab7b7c037a479e1cd2 Mon Sep 17 00:00:00 2001 From: Mark Farnum <46835237+farkmarnum@users.noreply.github.com> Date: Thu, 8 Aug 2024 14:39:21 -0400 Subject: [PATCH] Stop turf-mask mutating by default, make it an option (#2635) * Add a new test for the behavior we want * Add mutate option & default to cloning the mask input unless mutate is set to true. Also, add another test for this. * update docs * fix pnpm-lock.yaml * Update benchmark to include new `mutate` option * cleanup * Bringing recent TS version of mask into this PR. Should hopefully minimise conflicts required to be resolved before merging. * Missed adding @turf/clone to the prod dependencies. * Add new arg to type tests * resolve TypeScript error in bench.ts * Remove benchmark skipped input multi-polygon.geojson overlapping.geojson --------- Co-authored-by: James Beard Co-authored-by: mfedderly <24275386+mfedderly@users.noreply.github.com> --- packages/turf-mask/README.md | 9 ++++++- packages/turf-mask/bench.ts | 44 +++++++++++++++++++++++++++------ packages/turf-mask/index.ts | 20 ++++++++++++--- packages/turf-mask/package.json | 1 + packages/turf-mask/test.ts | 31 +++++++++++++++++++++++ packages/turf-mask/types.ts | 1 + pnpm-lock.yaml | 3 +++ 7 files changed, 97 insertions(+), 12 deletions(-) diff --git a/packages/turf-mask/README.md b/packages/turf-mask/README.md index 38aca306d6..1797d8c64a 100644 --- a/packages/turf-mask/README.md +++ b/packages/turf-mask/README.md @@ -11,6 +11,9 @@ ring polygon with holes. * `polygon` **([Polygon][1] | [MultiPolygon][2] | [Feature][3]<([Polygon][1] | [MultiPolygon][2])> | [FeatureCollection][4]<([Polygon][1] | [MultiPolygon][2])>)** GeoJSON polygon used as interior rings or holes * `mask` **([Polygon][1] | [Feature][3]<[Polygon][1]>)?** GeoJSON polygon used as the exterior ring (if undefined, the world extent is used) +* `options` **[Object][5]** Optional parameters (optional, default `{}`) + + * `options.mutate` **[boolean][6]** allows the `mask` GeoJSON input to be mutated (performance improvement if true) (optional, default `false`) ### Examples @@ -24,7 +27,7 @@ const masked = turf.mask(polygon, mask); const addToMap = [masked] ``` -Returns **[Feature][3]<[Polygon][1]>** Masked Polygon (exterior ring with holes). +Returns **[Feature][3]<[Polygon][1]>** Masked Polygon (exterior ring with holes) [1]: https://tools.ietf.org/html/rfc7946#section-3.1.6 @@ -34,6 +37,10 @@ Returns **[Feature][3]<[Polygon][1]>** Masked Polygon (exterior ring with holes) [4]: https://tools.ietf.org/html/rfc7946#section-3.3 +[5]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Object + +[6]: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Boolean + --- diff --git a/packages/turf-mask/bench.ts b/packages/turf-mask/bench.ts index a6f1aec249..f131561b37 100644 --- a/packages/turf-mask/bench.ts +++ b/packages/turf-mask/bench.ts @@ -1,13 +1,21 @@ -import { Feature, FeatureCollection, Polygon, MultiPolygon } from "geojson"; +import { FeatureCollection, Polygon, MultiPolygon, Feature } from "geojson"; import fs from "fs"; import path from "path"; import { fileURLToPath } from "url"; import { loadJsonFileSync } from "load-json-file"; import Benchmark, { Event } from "benchmark"; import { mask as turfMask } from "./index.js"; +import clone from "@turf/clone"; + +// type guard to narrow the type of the fixtures +const isPolygonFeature = ( + feature: Feature +): feature is Feature => feature.geometry.type === "Polygon"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const SKIP = []; + const suite = new Benchmark.Suite("turf-mask"); const directories = { @@ -17,6 +25,7 @@ const directories = { let fixtures = fs.readdirSync(directories.in).map((filename) => { return { + filename, name: path.parse(filename).name, geojson: loadJsonFileSync( path.join(directories.in, filename) @@ -24,15 +33,36 @@ let fixtures = fs.readdirSync(directories.in).map((filename) => { }; }); -for (const { name, geojson } of fixtures) { +for (const { name, filename, geojson } of fixtures) { + if (SKIP.includes(filename)) continue; + const [polygon, masking] = geojson.features; - suite.add(name, () => turfMask(polygon, masking as Feature)); + if (!masking || !isPolygonFeature(masking)) { + throw new Error( + "Fixtures should have two features: an input feature and a Polygon masking feature." + ); + } + + const getSuite = ({ mutate }: { mutate: boolean }) => ({ + name: `${name} (mutate = ${mutate})`, + fn: () => { + // We clone the inputs to prevent tests from affecting each other + turfMask(clone(polygon), clone(masking), { mutate }); + }, + }); + + suite.add(getSuite({ mutate: false })); + suite.add(getSuite({ mutate: true })); } -// basic x 4,627 ops/sec ±25.23% (21 runs sampled) -// mask-outside x 3,892 ops/sec ±34.80% (15 runs sampled) -// multi-polygon x 5,837 ops/sec ±3.03% (91 runs sampled) -// overlapping x 22,326 ops/sec ±1.34% (90 runs sampled) +/** + * Benchmark Results: + * + * basic (mutate = false) x 294,373 ops/sec ±0.25% (95 runs sampled) + * basic (mutate = true) x 307,397 ops/sec ±0.13% (97 runs sampled) + * mask-outside (mutate = false) x 100,575 ops/sec ±0.55% (97 runs sampled) + * mask-outside (mutate = true) x 103,180 ops/sec ±0.40% (94 runs sampled) + */ suite .on("cycle", (event: Event) => { console.log(String(event.target)); diff --git a/packages/turf-mask/index.ts b/packages/turf-mask/index.ts index 5d83ce3c94..43abb63043 100644 --- a/packages/turf-mask/index.ts +++ b/packages/turf-mask/index.ts @@ -7,6 +7,7 @@ import { } from "geojson"; import { polygon as createPolygon, multiPolygon } from "@turf/helpers"; import polygonClipping, { Geom } from "polygon-clipping"; +import { clone } from "@turf/clone"; /** * Takes polygons or multipolygons and an optional mask, and returns an exterior @@ -15,7 +16,9 @@ import polygonClipping, { Geom } from "polygon-clipping"; * @name mask * @param {Polygon|MultiPolygon|Feature|FeatureCollection} polygon GeoJSON polygon used as interior rings or holes * @param {Polygon|Feature} [mask] GeoJSON polygon used as the exterior ring (if undefined, the world extent is used) - * @returns {Feature} Masked Polygon (exterior ring with holes). + * @param {Object} [options={}] Optional parameters + * @param {boolean} [options.mutate=false] allows the `mask` GeoJSON input to be mutated (performance improvement if true) + * @returns {Feature} Masked Polygon (exterior ring with holes) * @example * const polygon = turf.polygon([[[112, -21], [116, -36], [146, -39], [153, -24], [133, -10], [112, -21]]]); * const mask = turf.polygon([[[90, -55], [170, -55], [170, 10], [90, 10], [90, -55]]]); @@ -27,10 +30,19 @@ import polygonClipping, { Geom } from "polygon-clipping"; */ function mask( polygon: T | Feature | FeatureCollection, - mask?: Polygon | Feature + mask?: Polygon | Feature, + options?: { mutate?: boolean } ): Feature { - // Define mask - const maskPolygon = createMask(mask); + const mutate = options?.mutate ?? false; // by default, do not mutate + + let maskTemplate = mask; + if (mask && mutate === false) { + // Clone mask if requested to avoid side effects + maskTemplate = clone(mask); + } + + // Define initial mask + const maskPolygon = createMask(maskTemplate); let polygonOuters = null; if (polygon.type === "FeatureCollection") { diff --git a/packages/turf-mask/package.json b/packages/turf-mask/package.json index 725b9ae3b4..c51057d707 100644 --- a/packages/turf-mask/package.json +++ b/packages/turf-mask/package.json @@ -64,6 +64,7 @@ "write-json-file": "^5.0.0" }, "dependencies": { + "@turf/clone": "workspace:^", "@turf/helpers": "workspace:^", "@types/geojson": "^7946.0.10", "polygon-clipping": "^0.15.3", diff --git a/packages/turf-mask/test.ts b/packages/turf-mask/test.ts index 51cf142bd0..7a94fd7a0a 100644 --- a/packages/turf-mask/test.ts +++ b/packages/turf-mask/test.ts @@ -12,6 +12,7 @@ import { fileURLToPath } from "url"; import { loadJsonFileSync } from "load-json-file"; import { writeJsonFileSync } from "write-json-file"; import { mask } from "./index.js"; +import { clone } from "@turf/clone"; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -48,6 +49,36 @@ test("turf-mask", (t) => { t.end(); }); +const getBasicPolygonAndMask = () => { + const basicFixture = fixtures.find( + ({ filename }) => filename === "basic.geojson" + ); + if (!basicFixture) throw new Error("basic.geojson not found"); + return basicFixture.geojson.features; +}; + +test("turf-mask -- doesn't mutate inputs by default", (t) => { + const [polygon, masking] = getBasicPolygonAndMask(); + const maskClone = clone(masking); + + mask(polygon, masking); + + t.deepEquals(masking, maskClone, "mask input should not be mutated"); + + t.end(); +}); + +test("turf-mask -- mutates mask input when mutate = true", (t) => { + const [polygon, masking] = getBasicPolygonAndMask(); + const maskClone = clone(masking); + + mask(polygon, masking, { mutate: true }); + + t.notDeepEqual(masking, maskClone, "mask input should be mutated"); + + t.end(); +}); + test("turf-mask polygon geometry", (t) => { // A polygon somewhere const polyCoords: Position[] = [ diff --git a/packages/turf-mask/types.ts b/packages/turf-mask/types.ts index 151727711a..be388c913d 100644 --- a/packages/turf-mask/types.ts +++ b/packages/turf-mask/types.ts @@ -22,3 +22,4 @@ const poly2 = polygon([ mask(poly1); mask(poly1, poly2); +mask(poly1, poly2, { mutate: true }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b6ba8f9ebc..06baabcf4a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -4231,6 +4231,9 @@ importers: packages/turf-mask: dependencies: + '@turf/clone': + specifier: workspace:^ + version: link:../turf-clone '@turf/helpers': specifier: workspace:^ version: link:../turf-helpers