[blockly] Adjust persistence blocks to breaking changes #2564
Conversation
Signed-off-by: Florian Hotze <[email protected]>
Signed-off-by: Florian Hotze <[email protected]>
Signed-off-by: Florian Hotze <[email protected]>
#1952 Bundle Size — 10.6MiB (+0.03%).Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #1952 |
Baseline #1949 |
|
|---|---|---|
 Initial JS |
1.86MiB |
1.86MiB |
| Initial CSS |
607.87KiB |
607.87KiB |
 Cache Invalidation |
17.91% |
0% |
| Chunks |
223 |
223 |
| Assets |
246 |
246 |
| Modules |
2876 |
2876 |
| Duplicate Modules |
149 |
149 |
| Duplicate Code |
1.86% |
1.86% |
| Packages |
95 |
95 |
| Duplicate Packages |
2 |
2 |
Bundle size by type 1 change 
1 regression
| Current #1952 |
Baseline #1949 |
|
|---|---|---|
| JS |
8.79MiB (+0.04%) |
8.78MiB |
| CSS |
890.75KiB |
890.75KiB |
| Fonts |
526.1KiB |
526.1KiB |
| Media |
295.6KiB |
295.6KiB |
| IMG |
140.74KiB |
140.74KiB |
| HTML |
1.24KiB |
1.24KiB |
| Other |
871B |
871B |
Bundle analysis report Branch florian-h05:blockly-persistence Project dashboard
Signed-off-by: Florian Hotze <[email protected]>
|
Tested together in a session with @florian-h05 in an online session and looks good to us. This is a BREAKING change that requires all Blockly rules that use persistence blocks to be reopened and saved again. |
@stefan-hoehn Do you have plans to do this? This will be another breaking change and I think we should try to do it before the release to have all changes done in one go. I will have to adapt my blocky rules working with persistence and quantity-types myself, but it will make it a lot cleaner afterwards. Currently I convert to text and add the unit atthe end again before updating an item. I just want to work with the values with the unit. Typed variables will also help for this. |
| case 'varianceSince': | ||
| case 'varianceUntil': | ||
| dayInfo = javascriptGenerator.valueToCode(block, 'dayInfo', javascriptGenerator.ORDER_NONE) | ||
| code = (isGraalJs) ? `${itemCode}.persistence.${methodName}(${dayInfo}${persistenceExtension})?.numericState` : `parseFloat(${persistence}.${methodName}(${itemCode}, ${dayInfo}${persistenceExtension}).getState())` |
There was a problem hiding this comment.
@stefan-hoehn This is inconsistent. For GraalJS, you focus on keeping backward compatibility by returning numericState. For Nashorn, it returns the full state.
Moreover, by returning numericState in GraalJS, you basically strip the unit and make it impossible to do straight quantity calculations on the result. I understand this would be a breaking change to remove this, but I think we should do it now rather than wait until after 4.2.
As an alternative, one could create extra methods (something like deltaStateSince). That would allow to keep backward compatibility for GraalJS at the expense of having different and less intuitive names in Blockly vs pure JS. I would prefer the breaking change here.
@florian-h05 What do you think?
There was a problem hiding this comment.
This is not inconsistent, for Nashorn there is also a numeric state returned (note the parseFloat around the state).
I have done this PR this way because after the breaking changes were done to code and the helper library, we needed to update Blockly quickly before the new milestone so it is not effectively „broken“.
Adjusting Blockly to allow use the new functionality was postponed to a later PR.
See #2595 (comment) for my implementation proposal.
This adjusts the persistence blocks for the breaking changes of
org.openhab.core.persistence.extensions.PersistenceExtensions.For NashornJS, see openhab/openhab-core#3736 for the changes.
For GraalJS, see https://next.openhab.org/addons/automation/jsscripting/#itempersistence for the current API docs and openhab/openhab-js#331 for the changes.
In a follow-up PR, Blockly should be extended to make use of the new capabilities provided by the changed return type of variance, deviation, average, sum & delta.