openhab / openhab-js

openHAB JavaScript Library for JavaScript Scripting Automation
https://www.openhab.org/addons/automation/jsscripting/
Eclipse Public License 2.0
38 stars 31 forks source link

Add sendCommand and postUpdate to `items` #232

Closed rkoshak closed 1 year ago

rkoshak commented 1 year ago

This continues an idea from https://github.com/openhab/openhab-webui/pull/1664#issuecomment-1413941287

The Problem

Compared to jRuby and Rules DSL, JS Scripting can be really verbose. While this isn't necessarily a problem by itself, it does become irksome when it comes to commanding and updating Items, largely because when all you have is the name of the Item you must pull the Item Object from items first.

Rules DSL:

sendCommand('ItemName', 'ON')

jRuby:

items["ItemName"].on
items["ItemName"].command ON.
items["ItemName"] >> ON

JS Scripting

items.getItem('ItemName').sendCommand('ON');
actions.BusEvent.sendCommand('itemName', 'ON');

This wouldn't be so irksome if sending a command or updating an Item you only have the name for is a rare occurrence. But in my case it's not. It's the most common case for me. And often I have to do it more than once in the same rule.

A Proposed Solution

This is by no means the only way to do this but my first thought is to add sendCommand('itemorname', 'string') and postUpdate('itemorname', 'string) to items.

items.sendCommand('itemName', 'ON');

I'm not tied to itemorname. I can see arguments for why we would want to only support name instead of the Item Object. But I like the idea that it would work with both.

Another Idea

Another idea which I've no idea if it's possible or if possible if it's an antipattern would be if items could behave like a dict too.

items['ItemName'].sendCommand('ON');

But that feels weird for it to be both a dict and an Object with functions and something tells me if this were feasible it would have been done this way to begin with. But if this makes sense, it would help make a number of other use cases a little more terse and easier to read.

The good news is either proposal would be non-breaking.

digitaldan commented 1 year ago

Sooo, when we first started porting this library over to be included by default, this was one of the first things i wanted to change. The problem was i struggled to find a good way to do this without feeling, well "dirty", and i decided i didn't want to impose my own personal preferences over having a clean API until others had an equal say in such matters. But since you bring it up ...... and since we now have more voices that can discuss, i think its a good time to revisit.

In my own personal system, i overide our default import with something like the following at the tops of scripts...

Object.assign(this, require('@dd'));

this dd.js includes looks like:

const ns = {};

// assign everything in @oh, so items, actions, triggers, etc....
Object.assign(ns, require("openhab"));

//alow dynamic getItem calls like items.F1_Master_Bedroom, but still support getItems and other default functions
ns.items = new Proxy(ns.items, {
    get: function (target, name) {
        return target[name] || items.getItem(name);
    }
});

module.exports = ns;

This allows me to call items.F1_Garage_Lights.sendCommand("OFF") , which you will notice mirrors the original rules DSL syntax. This of course pollutes the item namespace with a bunch of dynamic variables (item names), but is damn convenient and terse, and i greatly prefer its syntax. I also contemplated using another variable to hold these, like items.names.F1_Garage_Lights.sendCommand("OFF") but struggled if names or something even shorter would be appropriate.

I would LOVE to see something like this in the library, so i can retire my little workaround, but i appreciate others opinions as well

rkoshak commented 1 year ago

I really like that approach but it doesn't actually address my use case where all I've only got the name of the Item in a variable, right?

But there's no reason why we can't discuss both in this issue.

As you know, the whole concern of polluting the namespace doesn't bother me much (maybe if I had to use a bunch of third party NPM libraries I'd feel differently) so I'm not too concerned about that. The likelihood that an Item name and something already defined in items collides is probably exceedingly low. And it's all centralized in items and not polluting the rule itself so there shouldn't be too much chance of collisions with third party libraries anyway.

items is an OH thing, let's make it work the best way it can for OH stuff.

I also contemplated using another variable to hold these

Between the two I like it better without the names. But I wonder if that has implications for "autocompletion". I don't know well enough how that all works in MainUI and VS Code to talk intelligently on the topic but I could see it being a potential reason to choose one over the other.

Assuming that the dict idea from above, which could handle both your use case and my use case, isn't possible, I'd love to see your idea implemented. It will definitely bring JS Scripting more inline with the other languages. We can add the sendCommand and postUpdate methods to items to deal with my use case and I think the result over all would be an improvement.

Or would something like items.(event.itemName).postUpdate("OFF") work? I've still a lot to learn about JS. Then your solution would cover both use cases. :-D

If the dict idea is supportable, than we can hit both use cases I think because I can use a variable between the [ ] same as a string literal.

Once caution there though is items["itemname"] means something quite different in Nashorn which already causes confusion for some migrators.

digitaldan commented 1 year ago

I really like that approach but it doesn't actually address my use case where all I've only got the name of the Item in a variable, right?

items["ItemName"] would also work (i just did a quick test to verify as i was not 100% sure)

const ns = {};
ns.items = Object();
ns.items.bar = "foobar"
ns.items = new Proxy(ns.items, {
    get: function (target, name) {
        return target[name] || name;
    }
});

console.log(ns.items.foo) // outputs 'foo'
console.log(ns.items['foo']) // outputs 'foo'
console.log(ns.items.bar) // outputs foobar

Between the two I like it better without the names

Me too ;-)

ghys commented 1 year ago

This allows me to call items.F1_Garage_Lights.sendCommand("OFF") , which you will notice mirrors the original rules DSL syntax. This of course pollutes the item namespace with a bunch of dynamic variables (item names), but is damn convenient and terse, and i greatly prefer its syntax.

It's also the approach I took with widgets, as you know. Except in the widget's case you would only be served a simple object with a state field and eventually a displayState field. See https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui/web/src/js/store/modules/states.js

It could be nice to have a common approach to addressing items in JS, no matter if on the UI or in rules, like a common API, it could reduce a lot of confusion. In fact documentation about "an item" in JS could clearly mention "this method is only available in rules" and "this method is only available in UI expressions", but some basic stuff like items.<MyItemName>.state would be available in both.

florian-h05 commented 1 year ago

This allows me to call items.F1_Garage_Lights.sendCommand("OFF") , which you will notice mirrors the original rules DSL syntax. This of course pollutes the item namespace with a bunch of dynamic variables (item names), but is damn convenient and terse, and i greatly prefer its syntax.

The syntax looks good to me as well 👍

I also contemplated using another variable to hold these, like items.names.F1_Garage_Lights.sendCommand("OFF") but struggled if names or something even shorter would be appropriate.

I’d as well prefer to have it without the names.

FYI: I have just checked the code, and there is already such a functionality, but it‘s not working for me:

https://github.com/openhab/openhab-js/blob/81c413126fc39c432ed7ef2d38b88ec82d7b678f/items/items.js#L571-L586

The likelihood that an Item name and something already defined in items collides is probably exceedingly low.

I don‘t expect that there are much problems since it is unlikely that anyone names an Item exactly the same like a items method is named. (Who would name his Item getItem or even metadata?) If that‘s the case, it seems like the method still works, but the item cannot be accessed using the short way, which IMO is acceptable.

But I wonder if that has implications for "autocompletion". I don't know well enough how that all works in MainUI and VS Code to talk intelligently on the topic but I could see it being a potential reason to choose one over the other.

In VS Code auto-completion is based on the type definitions, I can check what gets auto-generated if we add that dict on the items namespace. UI auto-completion is using TernJS with a hand-written JSON definition so I don‘t expect there to be any problems (I guess @ghys knows better about it).

Once caution there though is items["itemname"] means something quite different in Nashorn which already causes confusion for some migrators.

@rkoshak I‘m not very familiar with Nashorn, could you explain me what it means there?

Seems like we all agree on items.<itemname> for short access to Items, correct?

digitaldan commented 1 year ago

FYI: I have just checked the code, and there is already such a functionality, but it‘s not working for me:

Huh, have no idea we had that there.

Seems like we all agree on items. for short access to Items, correct?

Yep!

florian-h05 commented 1 year ago

@digitaldan Do you want to contribute? You’ve already got the code from your helper function.

rkoshak commented 1 year ago

@rkoshak I‘m not very familiar with Nashorn, could you explain me what it means there?

In pure JSR223 (e.g. even in GraalVM if you require @runtime) items[] is a dictionary of Item Name/Current State. It's a short hand to get at an Item's state quickly without messing with the Item Object and such. Of course the key is a Java String and the value is the Java State so we don't want to use that in openhab-js as is.

I've seen some people migrating from Nashorn to JS Scripting wondering why their console.log(items['MyItem']); logs out Object, Object[] (or something like that) instead of the Item's current state.

Seems like we all agree on items. for short access to Items, correct?

Yes indeed. I wish I raised this before now. It's been bugging me for a long time and clearly it's been bugging you guys too since you've worked around it. :-D

digitaldan commented 1 year ago

@digitaldan Do you want to contribute?

Done.