This is a proposal (to @stefan.hoehn) of a small change within the behavior of Blocky contextual info blocks:
Currently if you use “contextual info” with parameter “received command”, “new State” or “previous State” it will become the following javascript code:
event.itemCommand.toString()
This will fail with an error if the rule is not triggered by a command (e.g. triggered manually, by a startup-event…) because “undefined” has no toString-method. To try put the following blockly in an empty rule and start it manually:
I think this problem could be avoided if the javascript-code for the block would be changed to:
event.itemCommand?.toString() || 'undefined'
In this case if itemCommand does not exist the expression would still return ‘undefined’ and the rule could be further executed.
Benefits:
No java-errors when starting rules manually during develpement
No java-errors when rules are started at system-startup (e.g. to avoid delay during first start of a rule after restart)
No java-errors if rule has different triggers (changes, commands etc.)
So my proposal would be to replace the three contextual-info-blocks that contain toString (previousState, newState and command) with the above expression. @stefan.hoehn : What do you think?
I like the proposal. However, I’m not sure I like using the String “undefined”. I think it would be better to use null.
Only if the rule doesn’t already expect a reasonable value. "undefined" isn’t a valid Item state or command (except for StringType). Therefore the user needs to test for it or else they might silently end up with weird results. Once they have to test for it, we may as well follow convention and use null to indicate no value instead of the String "undefined".
I think this is a case where it’s far better to generate an error when there is no value to actually work on then it is to silently run and end up with weird results. Consider if the rule is triggered by a Quantity and the first thing someone does is try to use a Quantity block to parse the event.newState with "undefined". You’ll still get an error.
Do you have a use case where a rule would be started with a system runlevel trigger and an Item event where a variable from event is used? In particular where the rule is triggered by a command? If the rule is triggered with and update or changed the Item’s state can be (and should be) used instead of the event value.
I don’t see how this can ever work unless the rule itself has if statements to detect what event triggered the rule and choose the right value from event in the first place. Once you have that you’ll know ahead of time whether the variable will be undefined or not and there is no need to keep test if the variable exists.
In short using "undefined" will mask a few errors but in many cases just move the error and it could silently cause weird results.
So I would propose one of the following (in no particular order):
File an issue on core (I think) to set the variable to null instead of leaving them as undefined. Users will have to test for it anyway and Blockly supports testing if a variable is null.
Add an undefined block so Blockly can test to see if a variable exists.
Change the JS code generated by the block to event.itemCommand?.toString() || null.
Note, the second or third option would also be useful in detecting if variables that are passed from other rules exist and doing something different if not.
Of course, that’s perfectly fine. I thought it matches better to the others as all the other contextual-info variants (triggering item, triggered channel…) already return undefined. But I know the discussions about null, undefined etc. so I’m fine with any solution.
[
A rule that has to be loaded at startup to avoid the delay when it’s first used. With jdk17 it’s only about 2sec, but for a switch it’s still a little long.
A rule that has to do jobs at startup and should react on commands. Of course you could split it up in two rules but then it’s not (easily) possible to share code.
In general: All the other contextual item blocks (triggering item, triggered channel…) already return undefined (It’s just the toString-command that’s included in the block (which is fine) that creates the error). For example the first one always works and returns undefined and the second (as it’s got the toString) breaks if no command available.
In general I think blockly should be low barrier whereas jdk-stack-traces could be a little scaring for beginners.
I know this is not entirely the same thing, but I work around it with a block in a utils block library:
uid: mherwege:blocky:utils
tags: []
props:
parameters: []
parameterGroups: []
timestamp: Jun 29, 2023, 2:27:42 PM
component: BlockLibrary
config:
name: Utils
slots:
blocks:
- component: BlockType
config:
args0:
- name: ITEMNAME
text: item
type: input_value
colour: 90
helpUrl: ""
message0: state or event state for %1
output: String
tooltip: Returns the state of item, or the new state from the event if the triggering item is the same as the item
type: item_name
slots:
code:
- component: BlockCodeTemplate
config:
template: >
{{utility:event_or_item_state}}({{input:ITEMNAME}})
utilities:
- component: UtilityFunction
config:
code: >-
function {{name}}(item) {
state = items.getItem(item).state;
if ((this.event !== undefined) && (event.itemName !== undefined) && (event.itemName === item)) {
state = event.itemState.toString();
}
return state;
}
name: event_or_item_state
This creates a block that returns the event state, or state of an item when the event state is undefined.
In this case the rule should check the trigger and exit if it’s a system started trigger. It doesn’t make sense for it to continue on processing with “undefined” instead of a real command.
But again, it would be better for the rule to check how it was triggered rather than continuing on processing with “undefined” instead of a real command.
In both of these cases, the rule needs to do something different based on how it’s triggered so it’s best to test the trigger to see what to do than it is to set the command to something that avoids an immediate error (“undefined”) but can result in errors later on or worse, no errors but incorrect behavior.
Right, but Blockly doesn’t provide a way to test for undefined. Any time you refer to a variable that doesn’t exist you will get undefined. Changing it to null at least gives Blockly a way to see if the variables can be used or not before trying to blindly use them. That’s why another alternative approach can be to create an undefined block that can be used in the same places as the null block.
It also gives a semi-convenient way to detect how the rule was triggered, though event.
Agreed but setting the value to to the string “undefined” when the variable doesn’t exist just moves where the stacktrace occurs much of the time. That’s my concern.
That’s a handy library for sure but it doesn’t help much in the cases where:
the received commands do not match the Item states with command triggers
the rule needs to do something different if the variables do not exist
Though testing event.type would be the proper way to handle that latter use case.
That’s a bad idea IMHO, as this would e.g. require to implement setting itemName to null in the ThingStatusTriggerHandler, and this class shouldn’t mess around with something like itemName, because its responsibility is to handle the ThingStatusTrigger, and not Item triggers.
Agreed
While the optional chaining is important, I don’t see what benefit having null as value instead of undefined should have. In both cases we need a new block to check for undefined or null.
That would be my preference as it has applications outside this specific use case. It should be super simple to implement but I don’t know if you can add it to “Logic” or if it’ll have to be under one of the openHAB categories. Since null is under “Logic”, that seems like the appropriate place for it but I don’t if there are technical or policy reasons to prevent that.
Another alternative would be somehow to make the null block cover both null and undefined but I don’t think that’s technically feasible while keeping the null block working like it does now.
Yet another alternative would be adding something like an “exists” block that tests for both undefined and null. Though this block would only really be useful with the two context blocks I think but that’s probably fine.
Would be nicer with return and a guard if-statement though …
From my POV, yes
That should work.
That would remove the burden from the user to differ between null and undefined, but I don’t know if that would be a good thing, since these two are different in their meaning.
Therefore I’d vote for the simple undefined block.
no, that was not the conclusion, it was just additionally.
Just the undefined block doesn’t help for what I described in my first post. The chain is important so that the rule does not fail.
I think we all agreed that it makes sense (although Rich thinks it would be better style to first check if an event element is available. While this might be correct the other way of directly testing if a command has a certain value should work too)
As I stated several times above, the chaining only keeps the rule from failing in a minority of cases, not all. And there are cases where the rule won’t fail but do the wrong thing.
Ultimately it is up to the rule developer to detect when a variable doesn’t exist and take appropriate action in that case. Implementing an undefined block provides an additional way to detect that.
if(event.newState == undefined) {
// do what needs to be done when it's undefined
}
else {
// do what needs to be done when it is defined
}
If you have to test anyway, why not test for undefined or test event.type?
If you are not testing it, then setting it to the string "undefined" in many/most cases is just going to move where the error and stack trace is generated, not actually solve the original problem.
That’s not true. It always prevents the rule from failing with a java error and only this was my point.
Because it’s not possible (as stated several times ). Because of the toString that is automatically appended to a contextual-info-block (with e.g. option command) it immediately breaks before testing against undefined. Just try it.
