@turf/helpers: Breaking change in 6.2.0 versus 6.1.4: coordinates must contain numbers

[jmealo]

6.2.0 reintroduces proper null-checking for points. Invalid usage of point throws errors where it had not in the past. If you found this issue by googling the error; double check that you're passing in your coordinates as longitude, latitude -- not latitude, longitude and check your usage carefully.

Error: coordinates must contain numbers
    at point (/Users/jeff/repos/find-nearest-us-cities/node_modules/@turf/helpers/dist/js/index.js:177:15)
    at /Users/jeff/repos/find-nearest-us-cities/index.js:17:25
    at Array.map (<anonymous>)
    at nearestCities (/Users/jeff/repos/find-nearest-us-cities/index.js:16:90)
    at Test.<anonymous> (/Users/jeff/repos/find-nearest-us-cities/test.js:12:24)
    at Test.bound [as _cb] (/Users/jeff/repos/find-nearest-us-cities/node_modules/tape/lib/test.js:90:32)
    at Test.run (/Users/jeff/repos/find-nearest-us-cities/node_modules/tape/lib/test.js:107:31)
    at Test.bound [as run] (/Users/jeff/repos/find-nearest-us-cities/node_modules/tape/lib/test.js:90:32)
    at Test._end (/Users/jeff/repos/find-nearest-us-cities/node_modules/tape/lib/test.js:201:11)
    at Test.bound [as _end] (/Users/jeff/repos/find-nearest-us-cities/node_modules/tape/lib/test.js:90:32)

[mfedderly]

This was actually done intentionally because these checks existed in 5.x but not 6.x. They were removed as a side-effect of converting the package to typescript. Although typescript can prevent the invalid points from being passed in, we still have javascript-only consumers that can benefit from the check. The check removal itself could have been a documented break from 5 to 6 but as far as I could tell it was not. One of my goals for 6.2.0 is to get users on the latest version from the 5.x series with as few breaking changes as possible, so I opted to restore this check.

Can you pre-check your points before calling point() or alternatively catch the error and handle it?

If there is an example point that shouldn't cause an error but does I'd love to add that as a test case and fix the logic.

As an aside, your calls to point have an array of [latitude, longitude] but turf expects [longitude, latitude]. I'm not sure how that affects the results of your code.

[jmealo]

@mfedderly: Thanks for your reply. The issue is that the error message is wrong and it thinks that no numbers are passed in for all cases (see npm test). I am passing in values, the tests pass with 6.1.4 and do not with 6.2.0. I forked this from a different script which had the values passed as documented and the results were always incorrect, when I reversed them, they became correct. It didn't make sense but correct results are correct results. I'm not sure if the data is getting flip/flopped during pre-processing, but the results are correct, I'm not sure if that's to be expected or not.

[jmealo]

@mfedderly: I checked, the flipped lat/lon doesn't impact distance calculations or the bug report. Thanks for pointing that out though. I'm not aware of any points that pass the current check.

Here are the coordinates from the tests that fail:

const latitude = 40.02456;
const longitude = -75.21407;

const latitude = 40.03871;
const longitude = -76.18218;

If it helps, this is using Node 14/15. Not sure if recent changes to numeric types broke the check.

[mfedderly]

I checked out your repository and set a breakpoint for the exception you are seeing. It appears that you have a bug on line 17 of test.js. You are expecting city to have .lon and .lat properties, but it is actually an array with 4 entries and not an object. You seem to correctly destructure this array on line 19.

You might also want to check your use of geokdbush, since that also appears to take longitude, latitude. Conceptually it is ordered that way because it is customary to have the x coordinate before the y coordinate. Most geospatial software uses that ordering even though it is different than the ordering that humans tend to use when speaking.

I'm going to go ahead and close this as the issue is in your code.

[jmealo]

@mfedderly: #1977

[jmealo]

@mfedderly: I don't know what tests you're referring to? These are the tests, which pass with 6.1.4 and fail with 6.2.0:

const test = require('tape');
const nearestCity = require('.');

test('find-nearest-cities', function (t) {

    t.test('nearest philadelphia city', function(t) {
        t.plan(3);

        console.time("benchmark");
        const latitude = 40.02456;
        const longitude = -75.21407;
        const cities = nearestCity(latitude, longitude, undefined, 5);
        console.timeEnd("benchmark");

        t.equal(cities[0].name, 'Manayunk', 'Philadelphia (NAME) should be the first item');
        t.equal(cities[1].name, 'Wissahickon', 'Philadelphia (NAME) should be the first item');
        t.equal(cities.length, 5, 'Should find at least 5 cities for Philadelphia.');
        t.end();
    });

    t.test('nearest city for Bird in Hand', function(t) {
        t.plan(1);

        console.time("benchmark");
        const latitude = 40.03871;
        const longitude = -76.18218;
        const cities = nearestCity(latitude, longitude, undefined, 1);
        console.timeEnd("benchmark");

        const city = cities[0]

        t.equal(city.name, 'Bird in Hand', 'Bird in Hand (NAME) should be the first item');
        t.end();
    });

});

It would appear that it's okay to invert lat/lon as long as you do it the same everywhere because the tests pass and are the results are correct.

[jmealo]

lon and lat don't appear in my tests...?

[mfedderly]

Oops, good call. The line numbers are for https://github.com/jmealo/find-nearest-us-cities/blob/master/index.js not test.js.

[jmealo]

@mfedderly: 🤦 Thank you so much, I'd love to buy you a beer/pizza. geokdbush.around already returns an ordered set. I changed the title of the ticket to help others out in the future. My correctness tests were passing and I conflated #1977 to mean that there was an issue with the check. Sorry that I filed this upstream too hastily; I thought my parameter checking and test coverage was sufficient.