feat: notifiers support in aries rest client js worker #1372
Conversation
| this.socket = new WebSocket(url); | ||
| this.socket.addEventListener('message', function (event) { | ||
| // TODO REST agents are not currently revealing topic information on incoming messages, |
There was a problem hiding this comment.
can you patch the REST agent?
There was a problem hiding this comment.
as part of #1323
All BDD tests will fail, so will push it in next PR.
| // TODO REST agents are not currently revealing topic information on incoming messages, | ||
| // Once REST supports this feature, topic value will be dynamic. | ||
| postMsg(newResponse("random-id", event.data, "", "all")); |
| } | ||
| stop(){ | ||
| this.socket.close(1011, "stopped notifier in aries-js-worker") |
There was a problem hiding this comment.
1011?
can we replace magic number with a named const?
There was a problem hiding this comment.
Also, you should patch REST agent to not throw a log on 1011 status code.
There was a problem hiding this comment.
There was a problem hiding this comment.
Never mind. Isn't 1011 an error code?(https://github.com/nhooyr/websocket/blob/94f9b71576625ef4dbe5950253cbfa8b539b88b5/close.go#L42)
Shouldn't you use a non-error status? Whichever status you use should be updated in REST server to ensure no error log is printed.
There was a problem hiding this comment.
Updated to normal closure code 1000
| @@ -106,7 +106,7 @@ | |||
| document.getElementById("mode-label").innerHTML = `Running in ${mode} mode` | |||
|
|
|||
| if (src == "rest"){ | |||
| document.getElementById("opts").innerHTML = `{"assetsPath": "/dist/assets", "agent-rest-url": "http://localhost:8082"}` | |||
| document.getElementById("opts").innerHTML = `{"assetsPath": "/dist/assets", "agent-rest-url": "http://localhost:8082", "agent-rest-wshook":"ws://localhost:8881"}` | |||
There was a problem hiding this comment.
the rest code called it a notifier rather than a hook. (not sure what is the correct generic term to use.)
There was a problem hiding this comment.
in agent-rest start command we call it webhook, so reused that term here.
notifier term is use for "startNotifier/stopNotifier" where we watch for incoming messages.
There was a problem hiding this comment.
Looking at these URLs, I see that we should document somewhere how to properly run this index.html
There was a problem hiding this comment.
ie. this goes way past just inflating the wasm file. It requires services up and running in addition to the inflated wasm.
There was a problem hiding this comment.
one other point: can we start using property names with no special characters such that the double quotes aren't needed? Like assetsPath for instance.
There was a problem hiding this comment.
updated docs
This new option requires a line in the docs
sudeshrshetty
force-pushed
the
WS-REST
branch
from
193dc14
to
f208fb8
Compare
|
This new option requires a line in the docs |
sudeshrshetty
force-pushed
the
WS-REST
branch
from
f208fb8
to
74d4f84
Compare
- new constructor opts `agent-rest-wshook` in rest client based aries js worker. If provided then `aries.startNotifier` can be used to listen to incoming messages - closes hyperledger-archives#1370 Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
sudeshrshetty
force-pushed
the
WS-REST
branch
from
74d4f84
to
e4d0b76
Compare
new constructor opts
agent-rest-wshookin rest client based aries jsworker. If provided then
aries.startNotifiercan be used to listen toincoming messages
closes Aries Rest Client JS worker - Add websocket notifier #1370
Signed-off-by: sudesh.shetty sudesh.shetty@securekey.com