[blockly] HTTP block enhancements #2607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@stefan-hoehn This is probably not for 4.2, so if you have time in the future, I would appreciate your help on trying to resolve the issue I still have with extra blocks being created. Issue has been resolved. |
#2206 Bundle Size — 10.83MiB (+0.05%).9330a10(current) vs 17c2119 main#2205(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #2206 |
Baseline #2205 |
|
|---|---|---|
 Initial JS |
1.89MiB |
1.89MiB |
| Initial CSS |
576.5KiB |
576.5KiB |
 Cache Invalidation |
17.97% |
17.79% |
| Chunks |
226 |
226 |
| Assets |
249 |
249 |
| Modules |
2914 |
2914 |
| Duplicate Modules |
149 |
149 |
| Duplicate Code |
1.8% |
1.8% |
| Packages |
96 |
96 |
| Duplicate Packages |
2 |
2 |
Bundle size by type 1 change 
1 regression
| Current #2206 |
Baseline #2205 |
|
|---|---|---|
| JS |
9.05MiB (+0.06%) |
9.04MiB |
| CSS |
862.92KiB |
862.92KiB |
| Fonts |
526.1KiB |
526.1KiB |
| Media |
295.6KiB |
295.6KiB |
| IMG |
140.74KiB |
140.74KiB |
| HTML |
1.35KiB |
1.35KiB |
| Other |
871B |
871B |
Bundle analysis report Branch mherwege:blockly_http Project dashboard
Generated by RelativeCI Documentation Report issue
I was able to make this work properly by using shadow blocks for all blocks that are being added, not trying to add/remove real blocks. I don't see issues with this approach. I took the same approach for the It is still not right: code generated when headers, query or payload dictionary is a varariable is not correct. |
|
I believe it now all works. I create a test calling out to https://postman-echo.com to test various combinations and log the results. This all looks fine. The code of the rule executing the various tests: A (partial) screenshot of the test script: And the test results when executing: |
mherwege
changed the title
|
I promise, it is the next thing I take care of 🙏🏻 |
|
In general it works but during my first tests I started with my own test case block which I already had. It looks like this: 
If you build this from scratch, all is good but if you start with the following already existing blocks
a mutator error is thrown. See the attached video. new-mutator-breaks-existing-block.movThe problem appeared pretty quick but it took me a while to reproduce it reliably. |
|
Strange that the error is thrown when moving the block away, and not already when adding the query. I will try to find what is causing this, may take some time though. Probably the right approach is to compare the code between your block, and the same block built from scratch. |
|
At least after lots of stepping into the code and commenting parts out that you have added, I was able to narrow it down to be related to lines 134ff
If you comment these out, no exception is thrown there and only don't know yet what the issue is here. Further if you only comment out the lower part and keep the upper part in it still throws the error. So the main culprit lies probably between 134 - 151. |
|
Another situation I detected: If you use GET instead of POST / PUT with the query block, you don't have the payload block and then you don't have that issue which makes sense because it is related to the payload section in the code! |
|
And this is the issue: I am just not sure why you did this? Additionally if you add logs, you can see that the content-Type detection is not working reliably when you change the dropdown selection. Now, I got the idea: No content: 
versus 
So basically the issue is the switch between having a field or an input. I am running out of time to fix it now but I hope that should be enough hints to solve it. 🤞🏻 |
I think you switched the message in the log. The first message should be 'NO Content', the second 'hasContent'. I used the appendDummyInput indeed to remove the input connection when there is no content (content type none). That also makes sure a block that is already connected to the input will get disconnected properly. If you do not do that, it stays connected and may be the wrong type. So the line is necessary. |
|
@stefan-hoehn I figured out what the issue was. The payload field was not added at the right position when switching from GET to POST (it should be after the query field, but it wasn't always). That should be fixed. I have not rebased yet. Let me know if I should do so. |
|
Great, good teamwork 😎 Yes, go ahead and rebase. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@stefan-hoehn I rebased and also simplified the code further. There were a number of fields that could be replaced by variables inside methods, reducing the complexity. From my perspective, it is ready for further testing. |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@stefan-hoehn What's the status here? Ready for my review? |
|
@florian-h05 my bad, I had actually notices Mark's chances but forgot about retesting it. Definitely go for a review now, while I will retest it! |
|
I retested the issue and the PR now looks good to me. @florian-h05 your turn :D |
florian-h05
reviewed
Aug 18, 2024
| this.appendValueInput('url') | ||
| .setCheck('String') | ||
| .appendField(imageTimeoutField, 'imgTimeout') | ||
| .appendField(imageHeaderField, 'imgHeader') | ||
| .appendField(imageQueryField, 'imgHeader') |
There was a problem hiding this comment.
I currently can‘t check how it looks like (only have my iPad with me), but is it correct that we have the imgHeader string here?
There was a problem hiding this comment.
No (even though it isn't really used to reference it later) - it should be imgQuery
There was a problem hiding this comment.
Indeed, should be changed, I didn’t notice in testing as the name reference is not used. I am not behind a PC, so difficult to do at the moment, but will do.
There was a problem hiding this comment.
@florian-h05 resolved now
| @@ -183,4 +266,23 @@ export default function (f7, isGraalJs) { | |||
| throw new Error(unavailMsg) | |||
| } | |||
| } | |||
|
|
|||
| function encodeParams (params) { | |||
There was a problem hiding this comment.
I would prefer to have this code inside something like a HTTP request builder inside openhab-js, but it‘s fine for me to merge this PR as is and adjust the code later once we have implemented this.
I will have limited time the next weeks so I won‘t implement that anytime soon (and hopefully don‘t forget that).
florian-h05
changed the title
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
|
@mherwege And finally a big "thank you" for the nice contribution! |
|
(btw, "we" should not forget to write some documentation about it 😜) |
|
Just getting acquainted and this is brilliant, thanks to @mherwege and all involved! My only suggestion would be to change the labels to something less technical like this:
|
Yes, may indeed be cleaner. I will see if I can do it without breaking already existing Blockly rules using it. @stefan-hoehn what do you think? |
florian-h05
pushed a commit
that referenced
this pull request
Sep 7, 2024
…2740) See #2607 (comment). Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based the nice work in #2411, I further enhanced this:
querymutator`: This mutator then allows adding query parameters in a dictionary. The values of the dictionary will be URL encoded and the query string constructed and appended to the url.HttpPostRequestandHttpPutRequestwith content typeapplication/json, allow any input type. For string input, the user is responsible to construct a valid JSON. For other input types (dictionary or any object), the code will construct a valid JSON object.HttpPostRequestandHttpPutRequestwith content typeapplication/x-www-form-urlencoded, rather than using a text input block, use a dictionary and also URL encode the parameters.application/jsonorapplication/x-www-form-urlencoded)Below are a few examples of the additional functionality.
Example 1: Query for
HttpGetRequestwhich generates:
Example 2:
HttpPostRequestwith content typeapplication/x-www-form-urlencodedwhich generates:
Example 3: combined with result assignment to variable:
which generates: