Skip to content
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

[@turf/clone] - object with key "length" is interpreted as array due to value.length in js code #1621

Closed
udos opened this issue Mar 17, 2019 · 6 comments · Fixed by #2749
Closed

Comments

@udos
Copy link

udos commented Mar 17, 2019

I have a GeoJSON feature with a key "length":

myGeoJSON = {
    geometry: [...],
    bbox: [...],
    properties: {
        processed: {
            length: {
                orig: 123.456,
                opti: 100.00,
            },
        }
    }
};

at some point, clone is called, which interprets the processed object as an array due to line

} else if (value.length) {

in

/**
 * Clone Properties
 *
 * @private
 * @param {Object} properties GeoJSON Properties
 * @returns {Object} cloned Properties
 */
function cloneProperties(properties) {
    var cloned = {};
    if (!properties) return cloned;
    Object.keys(properties).forEach(function (key) {
        var value = properties[key];
        if (typeof value === 'object') {
            if (value === null) {
                // handle null
                cloned[key] = null;
            } else if (value.length) {
                // handle Array
                cloned[key] = value.map(function (item) {
                    return item;
                });
            } else {
                // handle generic Object
                cloned[key] = cloneProperties(value);
            }
        } else cloned[key] = value;
    });
    return cloned;
}

and throws an error...

looking at the TypeScript code, it looks as this is handled properly (source: https://github.com/Turfjs/turf/blob/master/packages/turf-clone/index.ts#L78):

} else if (Array.isArray(value)) {

there might be a reason for this, but it would be nice if JS array identification would be done in a more robust way.

p.s.: meanwhile I change my key :|

@rowanwins
Copy link
Member

Thanks for the report @udos

@rowanwins
Copy link
Member

I've just done a quick check and I'm having trouble replicating your error
https://runkit.com/rowanwins/clone-issue

Another option to check out is the v5 series of modules was published using regular js so there shouldn't be any ts to js transformation issues there.

Hope that helps

@udos
Copy link
Author

udos commented Mar 18, 2019

@rowanwins, I tried to reproduce on runkit but I'm struggling to understand how to use it properly :|

is it possible that nodejs is using the ts files where the Array identification is properly identified?

note: I'm using the latest versions via npm. actually I'm using @turf/simplify which at some point calls @turf/clone when the error is thrown.

from your runkit I noticed that @turf/clone is at version 6.0.2. @turf/simplify is at version 5.1.5. @turf/clone is a dependency of @turf/simplify (package-lock.json):

"@turf/simplify": {
      "version": "5.1.5",
      "resolved": "https://registry.npmjs.org/@turf/simplify/-/simplify-5.1.5.tgz",
      "integrity": "sha1-Csjyei60IYGD7dmZjDJ1q+QIuSY=",
      "requires": {
        "@turf/clean-coords": "^5.1.5",
        "@turf/clone": "^5.1.5",
        "@turf/helpers": "^5.1.5",
        "@turf/meta": "^5.1.5"

so @turf/clone is "locked" at version 5.5. I will try to re-install @turf/simplify to see if this changes.
npm outdated did not indicate me outdated turf components...

@rowanwins
Copy link
Member

As yes ok with v@5.1.5 of clone I'm seeing the issue, looks like the offending code is here . It looks like it was resolved last year in the v6 series here.

The challenge you'll have is that @turf/simplify hasn't been re-released at a v6 module which means the changes to clone haven't been incorporated :(

@udos
Copy link
Author

udos commented Mar 20, 2019

thanks. no worries!

the workaround until @turf/simplify is re-released as a v6 module could be that I uninstall @turf/simplify, install @turf/clone 6.0.2 (so it is available as a package and not as a dependency) and then re-install @turf/simplify. @turf/simplify might then use @turf/clone 6.0.2...

I will give that a try later today

@smallsaucepan
Copy link
Member

Fairly sure this would have been resolved when Turf updated its JS target from es5 (as of v5.1.5) to es20xx. Have confirmed the currently generated code for CJS and ESM builds uses the Array.isArray call rather than the value.length check.

Have submitted PR #2749 to add a regression test making sure a property with a name length makes it through cloning unscathed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants