Except when you try to send “undefined” as a command or update to anything that’s not a String Item. Or if you try to pass “undefined” to a Quantity block or otherwise do math with it.
Indeed, I forgot about that too. The chaining is still required but the “undefined” needs to just be undefined now that we have a block to compare to.
We need both changes.
So can you both summarize what exactly you want me to do?
Sure, I try (don’t know why things got so complicated):
if (type === 'asNumber') {
return [`parseFloat(event.${contextInfo}?.toString()|| undefined)`, javascriptGenerator.ORDER_ATOMIC]
} else if (type === 'asQuantity') {
return [`Quantity(event.${contextInfo}?.toString()|| undefined)`, javascriptGenerator.ORDER_ATOMIC]
} else {
return [`event.${contextInfo}?.toString()|| undefined`, javascriptGenerator.ORDER_ATOMIC]
}
Yes, it’s just removing the quotes from around “undefined” in the original proposal.
Stuff like this has huge overall impact which makes it really hard to change Blockly later if we get it wrong. It’s important to think of all the angles.
The undefined fallback is a really bad idea IMO.
The optional chaining will directly evaluate to undefined if e.g. itemName is undefined (see Optional chaining (?.) - JavaScript | MDN). The additional undefined fallback will make the expression undefined if the string value of itemName is falsy, which means that e.g. a Item state of 0 would evaluate to undefined, which is not correct.
Adding the optional chaining is enough, the undefined fallback is not needed and harmful.
@Larsen If you agree to Florian I would then quickly do the PR.
I just want to make sure that we are covering the original reported problem though.
If you use the Five Why’s method we are lead back to the root cause of the problem reported in this thread is that in the UI, the event Object is the Java event object and all the variables in it are Java Objects, not JS.
In order to avoid problems trying to, for example, comparing an OnOffType (which is what event.newState et al would be carrying) to the JS String "ON", Blockly adds a toString() call when ever it retrieves anything stored in event.
The problem comes in when that variable doesn’t exist. The call to toString() fails with a stack trace that is meaningless to the end users. That was the original problem in this thread.
We’ve added the undefined block which is great! It has uses outside of this specific problem. However, if we keep the call to toString() without a fallback we’ve not actually solved this problem. There is no way for Blockly users to actually test whether a variable exists in event prior to trying to use it and if they do use it they get an incomprehensible error in the logs.
If we get rid of the call to toString() we’d be able to test for undefined but wouldn’t be able to use that variable without further work to turn it into a String for comparisons.
We need a solution that both allows the user to test for undefined prior to use and not have to convert the event variables to a String themselves.
Optional chaining solves this without having to provide the undefined fallback after the || OR.
event.newState?.toString()
// equivalent to:
event.newState === undefined ? undefined : event.newState.toString()
// not equivalent to:
event.newState?.toString() || undefined
// because if newState.toString() is e.g. empty it has a falsy value,
// this expression will return undefined, which is wrong:
// Empty string state is not the same as undefined
Misunderstanding: The chain is the fallback. Just like the code I proposed in #20. And the proposal is already without quotes.
do I understand this right: You don’t even need the “|| undefined” to realise the chain?
Yes, that’s what I learned from @florian-h05