-
Notifications
You must be signed in to change notification settings - Fork 934
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
Support Foreign Members to @turf/clone #904
Conversation
packages/turf-clone/index.js
Outdated
if (geojson.bbox) cloned.bbox = geojson.bbox; | ||
|
||
// Custom Properties | ||
Object.keys(geojson).forEach(function (key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere doesn't this in practice replace the cloneAll
feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe.. but I don't think so?... pretty sure cloneAll
feature clones all property values (JSON.parse + JSON.stringify), so if you need to mutate some of the properties then it might be best to use that option.
However.. we could test it out and maybe it can replace the cloneAll
feature entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cloneAll
is primarily used to clone properties
, so we could test for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to preserve the properties
using the default setting, so I've dropped that the cloneAll
param. Doesn't really have any backwards compatibility effects since it behaves the same way if true
or false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere what if the user wants still to clone only the GeoJSON object (no foreign members) taking advantage of a higher speed? I'd keep an optional parameter here, like geojsonOnly
or something, to allow that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 ... it's really not a big speed difference speed.
The bigger difference is mutate=true
to not even do this clone
operation at all!
The new "foreign member" support acts also to support id
& bbox
which is apart of the GeoJSON spec, so we shouldn't make that optional, we should clone those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then now this is just a faster version of JSON.parse(JSON.stringify(thing))
! 😜
And it makes sense to expect that the whole thing
is going to be cloned.
Good job! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! It's a faster JSON.parse(JSON.stringify(geojson))
tool, also it looks "cleaner" by just wrapping it with clone(geojson)
.
It's about 2-3x faster... depends on the type of Geometry.
packages/turf-clone/index.js
Outdated
// Custom Properties | ||
Object.keys(geojson).forEach(function (key) { | ||
if (['id', 'type', 'bbox', 'features'].indexOf(key) !== -1) return; | ||
cloned[key] = geojson[key]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere does this guarantee you can modify the properties of the cloned object without changing the original ones? Just want to make sure this issue doesn't happen again 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have any impact, before these properties were being dropped (so it's better than before).
We can add some tests for this behavior.
It shouldn't have any impact on the issue you mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no impact on that specific issue; I just used that as an example of creating a reference to the item instead of cloning it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we just need to test if those properties get mutated or not.
What's been done so far:
|
Going to merge, if we find any other issues, I'll open up a new PR with tests/solution. |
Slightly outside the scope of GeoJSON, Foreign members are members which is not described by the GeoJSON specification (https://tools.ietf.org/html/rfc7946#section-6.1).
Tasks
cloneAll
since the default does the same thing asJSON.parse/stringify
id
&bbox
Before
Using
@turf/clone
would remove all foreign members by default, unless you performed theJSON.parse + JSON.stringify
combo which is significantly slower.Now
By default all members are preserved including
id
&bbox
.