W3C compliance - Use forms instead of links in Properties, Actions, and Events #2811
Conversation
|
I'm having problems running this: These look like TypeScript errors to me (I'm new to TypeScript). Maybe I didn't link the dependency correctly though? |
|
Yep, it is our friendly typescript compiler. Did you build the gateway-addon? sorry I forgot to mention it, my bad. |
|
This code all looks good to me so far, thanks. Not sure if @tim-hellhake wants to take a look from a TypeScript point of view since that's new to me? In testing this I installed the virtual things adapter and noticed lots of type errors in the console like the one below: I was able to add a virtual thing, but trying to set properties results in a 400 error with the payload
It looks like virtual-things-adapter might need to be updated :/ I haven't tested it with any other adapters yet. |
|
For the record, in addition to the steps above, I needed to build gateway-addon-node with a custom build command which doesn't do a submodule update and switch away from @relu91's w3c-compliance branch: before doing the npm link step. |
|
Some updates on this PR. I tried to figure out the impact of changing The report was then manually reviewed to eliminate false positives. So currently I find out 9 addons that should be updated after merging this PR. The addons marked as Since I am aware of the weaknesses of this process I would rate the different marks as follows:
|
|
That's extremely useful, thank you @relu91. So my understanding is that if we want to rename links to forms at the IPC layer as well as the web layer, the following add-ons would be impacted:
This seems like good news to me. @flatsiedatsie @bewee @tim-hellhake would you be willing to rename "links" to "forms" in your add-ons before 2.0 is released? We can make the change to the web layer only, but it would be much better to have it be consistent all the way down the stack. We'd need to figure out an update strategy so that add-ons keep working while the upgrade is still rolling out. |
|
Ofcourse, I'd be happy to. Would the addons still work for people on the old version of the gateway is the addon started using the new names? |
I think it depends on addons maintainers. If in the logic of your addon you want to maintain backward compatibility you can easily check for links or forms and behave differently. There is no other change in the IPC layer so I think they will just continue to work. But @benfrancis has the final word here :) |
If this is straightforward then it would be a good practice to recommend, so that add-ons can work with both 1.x and 2.x until 2.x becomes more widely adopted. |
|
@benfrancis Sure! |
|
@relu91 @benfrancis |
|
@relu91 @benfrancis |
|
@tim-hellhake Thanks! let me update the dependency and see if the cli is ok with it 👍🏻 |
|
I tried to dig a little bit about why on node 10 we go a timeout in the tests. Locally, I can sometimes reproduce the issue. It seems that the virtual-things-adapter once installed it causes problems in the clean-up procedures. In particular, AFIK the |
Codecov Report
@@ Coverage Diff @@
## master #2811 +/- ##
==========================================
+ Coverage 65.37% 65.43% +0.06%
==========================================
Files 124 124
Lines 7971 7986 +15
Branches 1317 1315 -2
==========================================
+ Hits 5211 5226 +15
Misses 2718 2718
Partials 42 42
Continue to review full report at Codecov.
|
|
Note: While I'm waiting for #2871 to land I've been continuing work on the links -> forms change in https://github.com/benfrancis/gateway/tree/links-to-forms which also includes those changes. |
relu91
force-pushed
the
fix_2806
branch
2 times, most recently
from
2dfc982
to
0c8fe9b
Compare
fixes WebThingsIO#2806 Co-authored-by: Ben Francis <[email protected]>
There was a problem hiding this comment.
A couple of nits and I have one concern about how the forms are selected in the front end. I'm also going to suggest a follow-up to add backwards compatibility for Thing Descriptions which use links instead of forms so that we don't break existing add-ons. But otherwise I think this is very close to being landed!
| if (action.links) { | ||
| action.links = action.links | ||
| .filter((link) => { | ||
| return link.rel && link.rel !== 'action'; | ||
| }) | ||
| .map((link) => { | ||
| if (link.proxy) { | ||
| delete link.proxy; | ||
| link.href = `${Constants.PROXY_PATH}/${encodeURIComponent(this.id)}${link.href}`; | ||
| } |
There was a problem hiding this comment.
Following our discussions yesterday I'm going to suggest a follow-up to re-visit this later and add in backwards compatibility for links, by translating to forms at this point.
|
I've filed a follow-up issue regarding backwards-compatibility #2881 |
|
I've also filed a follow-up for the WebSocket endpoint, which I've realised is currently still a link #2882 |
| throw error; | ||
| } | ||
| })?.href; | ||
| } |
There was a problem hiding this comment.
Is this file the right place to introduce this utility function?
I use the spread operator and the reverse function, although it might be not so efficient for a large forms array I think it is more expressive than a for loop that searches the form backward.
About the selection logic, tell me if you find it satisfactory. I choose to check also for protocol, maybe is an overkill? wot-adapter may publish TDs with other protocols in forms..
There was a problem hiding this comment.
I also wanted to provide unit tests for this particular function, but it is not easy to test the frontend code so I gave up. Feel free to suggest how to properly test this.
There was a problem hiding this comment.
Is this file the right place to introduce this utility function?
If it was only needed inside thing-model.js I would have put it there since this logic is quite specific to parsing a Thing Description, but since it's needed in multiple files I think it's reasonable to put it in this central utilities file.
I use the spread operator and the reverse function, although it might be not so efficient for a large forms array I think it is more expressive than a for loop that searches the form backward.
About the selection logic, tell me if you find it satisfactory. I choose to check also for protocol, maybe is an overkill? wot-adapter may publish TDs with other protocols in forms..
I think what I would have done is to just traverse the array forwards looking for matches and pick the last result, since there could theoretically be multiple matches, but I think your approach has the same effect. As far as I know the Thing Description specification doesn't specify what to do if there are multiple forms for the same operation using the same protocol.
I agree that in theory we should check the protocol, but if you're already making the assumption that the last matching form is an endpoint added by the gateway, then you could possibly also assume that it will be using http/https since that's all the gateway currently exposes.
static/js/models/thing-model.js
Outdated
| @@ -216,7 +217,7 @@ class ThingModel extends Model { | |||
| if (typeof this.propertiesHref === 'undefined') { | |||
| const urls = Object.values(this.propertyDescriptions).map((v) => { | |||
| if (v.forms) { | |||
| return v.forms[0].href; | |||
| return Utils.selectFormHref(v.forms, Constants.WoTOperation.WRITE_PROPERTY); | |||
There was a problem hiding this comment.
I think for extensibility it might be right to look for writeproperty operation here even if now it is just reduntat.
| break; | ||
| } | ||
| } | ||
| const href = Utils.selectFormHref(action.forms, Constants.WoTOperation.INVOKE_ACTION); |
There was a problem hiding this comment.
For consistency, I always used selectFormHref when looking for the gateway form.
There was a problem hiding this comment.
This looks good, but I suggest either removing the protocol check or first resolving href against base before checking for http: or https:. (See further comments inline).
I think we could make this all more efficient by parsing the forms once on instantiation and storing a normalised model of the thing's affordances which has a URL endpoint per operation. Then we don't need to parse the forms every time we want to use them. However, this could be part of wider work to consolidate the Thing Description parsing logic and I'm happy to file a follow-up issue for that.
static/js/utils.ts
Outdated
| try { | ||
| const { protocol } = new URL(selectedForm.href); | ||
| return ( | ||
| protocol === 'http:' && |
| throw error; | ||
| } | ||
| })?.href; | ||
| } |
There was a problem hiding this comment.
Is this file the right place to introduce this utility function?
If it was only needed inside thing-model.js I would have put it there since this logic is quite specific to parsing a Thing Description, but since it's needed in multiple files I think it's reasonable to put it in this central utilities file.
I use the spread operator and the reverse function, although it might be not so efficient for a large forms array I think it is more expressive than a for loop that searches the form backward.
About the selection logic, tell me if you find it satisfactory. I choose to check also for protocol, maybe is an overkill? wot-adapter may publish TDs with other protocols in forms..
I think what I would have done is to just traverse the array forwards looking for matches and pick the last result, since there could theoretically be multiple matches, but I think your approach has the same effect. As far as I know the Thing Description specification doesn't specify what to do if there are multiple forms for the same operation using the same protocol.
I agree that in theory we should check the protocol, but if you're already making the assumption that the last matching form is an endpoint added by the gateway, then you could possibly also assume that it will be using http/https since that's all the gateway currently exposes.
static/js/utils.ts
Outdated
| ); | ||
| } catch (error) { | ||
| if (error instanceof TypeError) { | ||
| // URL is relative or not well formatted |
There was a problem hiding this comment.
I think I'm right in saying that all the form hrefs added by the gateway are just paths rather than absolute URLs (e.g. /things/foo/properties/on). That means this logic would always be triggered and the protocol check would never be applied.
Rather than just ignore the protocol if not an absolute URL, I think if you're going to check the protocol what you need to do here is resolve the href against the base member of the Thing Description first.
This is probably strictly not necessary since the last form entry (added by the gateway) will always be HTTP, so we could just remove the protocol check if you prefer.
|
Thank you Ben I had the same concerns. I kept the protocol check, it feels more bulletproof to me but we can revisit later on. What do you of the current status?
Yes, I was tempted of doing the same but since this is already a pretty big change I wanted to not bring too much to the table. I follow-up PR would be perfect. |
|
I've been trying Gateway 1.1.0, and have run into this change. Some of my adons look for the I noticed that there is not a forms array. But the links array is also still there, just.. empty. Perhaps the gateway could populate both the forms and links array? Then it would remain backwards compatible with these addons. |
This PR covers the changes described in #2806 but it still does not tackle the open questions. It is merely a refactor of the current code base divided into small commits. I also introduced a test to verify the correct behavior of the Camera component in the Web UI.
The PR is still in the draft because we still need to understand how much this change will impact the addons and how to cope with the root level forms (for further details refer to #2806).
Note this change requires the updated version of https://github.com/WebThingsIO/gateway-addon-node in my forked repository. If you want to test this PR locally do the following: