-
Notifications
You must be signed in to change notification settings - Fork 943
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.
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
v6.5.0 introduced a breaking change to turf.mask
: it now mutates the mask input
#2634
Comments
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. |
@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? |
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 -- #693 |
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 I noticed that in 6.2.0 line-to-polygon was changed to no longer mutate by default, but rather use the option. |
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
|
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! |
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 - |
@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? |
@smallsaucepan fine by me! |
I believe it was unintentional to change the default behavior here. |
On version
v6.5.0
,turf.mask(polygon, mask)
now mutatesmask
in place. In versionv6.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:
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 tofalse
(liketurf-simplify
does). I'll make a PR with that approach.The text was updated successfully, but these errors were encountered: