peter-murray / node-hue-api

Node.js Library for interacting with the Philips Hue Bridge and Lights
Apache License 2.0
1.19k stars 145 forks source link

Fix ListType#getValue #134

Closed gcandal closed 4 years ago

gcandal commented 4 years ago

Running on Node v11.13.0, the following call results in an error:

api.setLightState(id, {
     on: true,
     xy: [0.1948, 0.5478]
})

Similar to:

    "stack": [
        "Error: Value, 'function(obj) {\r",
        "        var isObject = typeof obj === \"object\";\r",
        "        if (!isObject) {\r",
        "            return this.indexOf(obj);\r",
        "        }\r",
        "\r",
        "        for (var index = 0; index < this.length; index++) {\r",
        "            var valid = true;\r",
        "            var _obj = this[index];\r",
        "\r",
        "            for (var key in obj) {\r",
        "                var value = obj[key];\r",
        "                var _value = _obj[key];\r",
        "                if (value === _value) {\r",
        "                    continue;\r",
        "                }\r",
        "                valid = false;\r",
        "                break;\r",
        "            }\r",
        "\r",
        "            if (!valid) {\r",
        "                continue;\r",
        "            }\r",
        "\r",
        "            return index;\r",
        "        }\r",
        "\r",
        "        return -1;\r",
        "    }' is not within allowed limits: min=0 max=1",
        "    at FloatType.getValue (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/parameters/RangedNumberType.js:43:15)",
        "    at ListType.getValue (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/parameters/ListType.js:49:34)",
        "    at LightState._setStateValue (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/bridge-model/lightstate/States.js:87:53)",
        "    at Object.keys.forEach.key (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/bridge-model/lightstate/States.js:72:16)",
        "    at Array.forEach (<anonymous>)",
        "    at LightState.populate (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/bridge-model/lightstate/States.js:70:25)",
        "    at getStateForDevice (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/api/http/endpoints/lights.js:132:16)",
        "    at Object.buildLightStateBody [as _payloadFn] (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/api/http/endpoints/lights.js:106:19)",
        "    at ApiEndpoint.getRequest (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/api/http/endpoints/endpoint.js:101:26)",
        "    at Transport.execute (/Users/gcc/ripe-sputnik-device/node_modules/node-hue-api/lib/api/http/Transport.js:43:33)"
    ]

Where the problem is that the for-loop is iterating over Array properties, and not only its actual elements.

peter-murray commented 4 years ago

I am not seeing this in Node.js 10 or 12, am yet to get node 11 installed to test this.

I have created a test case where I am using a raw Javascript object in place of the LightState

hue.lights.setLightState(id, {on: true, xy: [0.1948, 0.5478]});

And I do not get an error under Node.js 10 or 12. In both cases in the debugger, I have the array of [0.1948, 0.5478] and the idx is in fact the index of the item in the array, 0 or 1.

Can you please double check your calling code that generates the error stack you provided and give an exact test case that can reproduce this or a code sample that will do it so I can investigate this fully, as I have concerns that I cannot reproduce this and for every Issue, it needs to be backed by failing tests that I can validate against.

gcandal commented 4 years ago

Hi Peter, first of all thanks for the quick response.

The exact call that I can reproduce the error with is:

require("node-hue-api").v3.api.create(HOST, USER, TIMEOUT).lights.setLightState(
    2,
    {
        on: true,
        bri: 254,
        colormode: "xy",
        xy: [0.153, 0.048]
    }
)

I didn't mention it before but I'm running the latest node-hue-api (3.1.3) and using MacOS (don't know if that might be relevant, guess not).

Also, I've tried the same thing on Node v10.16.3 without this patch and the error persists.

peter-murray commented 4 years ago

Can you please try version 3.1.4 that has been published to the npm registry to see if that fixes the issue for you.

I still could not recreate it under a Windows host, but will test tomorrow on MacOS to see if I can reproduce the problem.

gcandal commented 4 years ago

It's fixed 👍

peter-murray commented 4 years ago

That is great news, now I need to work out what is the difference in the test cases as to why it would not fail for me as I need to cover this to prevent the issue from cropping up again.

Thanks for confirming the fix.