v6.5.0 introduced a breaking change to `turf.mask`: it now mutates the mask input

[farkmarnum]

On version v6.5.0, turf.mask(polygon, mask) now mutates mask in place. In version v6.3.0, it did not.

Since this change is not documented, it can lead to some hard-to-diagnose bugs.

Here's a simple test to verify:

const poly1 = turf.polygon([[[-73.2, 42],[-73.2, 42.2],[-73, 42.2],[-73, 42],[-73.2, 42]]]);
const poly2 = turf.polygon([[[-73.1, 42],[-73.1, 42.2],[-73.3, 42.2],[-73.3, 42],[-73.1, 42]]]);
const poly2Clone = turf.clone(poly2);
const _masked = turf.mask(poly1, poly2);
console.assert(turf.booleanEqual(poly2, poly2Clone)); // passes on v6.3.0, fails on v6.5.0

This is due to the changes in #2130, which rewrote turf.mask().

The simplest solution is probably to add a mutate option and default it to false (like turf-simplify does). I'll make a PR with that approach.

[twelch]

Hi @farkmarnum it's unfortunate a change was not documented. Can you confirm what the new version 7.0 turf.mask behavior is? .The simplest approach is probably just to document the existing behavior in the JSDoc comments for the function (ends up published in the API docs). If you do put together a code PR as described, I think it would be welcome, no reservation here. And I would be curious the difference.

[farkmarnum]

@twelch just checked and the behavior on 7.0.0 is the same (the mask input is mutated). I've opened a PR to fix here: #2635

[smallsaucepan]

@twelch and @rowanwins do we know if the mutating behaviour introduced in 6.5.0 was intentional? The gist of that PR seems to be performance related, so maybe it wasn't?

[twelch]

I'm open to hearing straight from Rowan. What I've read is that mutating was not part of the original design/spirit and later became opt-in for a few functions. I'm not sure if that's held true since -- https://github.com/Turfjs/turf/issues/693

[twelch]

Some additional thoughts. Whether intentional or not, the precedent in the code seems to be mutation as an opt-in option, not a default behavior. At first glance I see the mutate option available (for performance reasons) for truncate, simplify, transformScale, transformRotate, rewind, projection, turf-line-to-polygon, flip, polygon-dissolve, cluster-kmeans, dissolve, concave.

I noticed that in 6.2.0 line-to-polygon was changed to no longer mutate by default, but rather use the option.

[farkmarnum]

I just updated the benchmark code in that PR as well. Interestingly, there doesn't seem to be much of a performance difference, just a small perf boost when mutate = true:

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)

[smallsaucepan]

Agree mutation should be opt in. If unintentional, we can fix straight away as a bug. If it was intentional, and just not documented, that makes it a bit murkier whether it's a breaking change.

Let's see how good @rowanwins memory is!

[smallsaucepan]

Haven't heard any further on this. From looking at the tests, am guessing the new behaviour wasn't intentional.

Will do a comparison with a large file to see which works best - ~6.5.0~ 6.3.0 code as it was, or current code with a clone by default. From there we can decide whether to roll back or push ahead with this PR. How does that sound @farkmarnum and @twelch?

[smallsaucepan]

@farkmarnum since you started on this we ported mask to Typescript and I don't think it's fair you should have to merge all that in. Are you happy if I commit your fix merged into the current TS implementation to your PR branch?

[farkmarnum]

@smallsaucepan fine by me!

[mfedderly]

I believe it was unintentional to change the default behavior here.