updating shape of websocket api for at-driver draft spec #31
Conversation
gnarf
requested review from
mzgoddard and
jugglinmike
and removed request for
jugglinmike and
mzgoddard
test/test.js
Outdated
| const {whenClosed} = await run([]); | ||
| return Promise.race([whenClosed, invert(connect(4382))]); | ||
| }); | ||
| // test('rejects unspecified protocol', async () => { |
There was a problem hiding this comment.
@jugglinmike I might be completely missing the relevant point in the spec, but not sure this is a check we need anymore with sub-protocols
There was a problem hiding this comment.
Should we use test.skip?
There was a problem hiding this comment.
I chose to comment because we should either re-enable it because something about it should be tested or remove it entirely. Just wasn't sure which without asking
There was a problem hiding this comment.
Thanks, @gnarf. Although AT Driver is a ways off from being a standard, I think we're past the point where explicit versioning is helpful. I'll remove these tests.
| return { | ||
| sessionId: websocket.sessionId, | ||
| capabilities: { | ||
| atName: 'TODO', |
There was a problem hiding this comment.
Not sure how much we want to do with capabilities here - but the harness does use this information so @mzgoddard will want it to be accurate
There was a problem hiding this comment.
Yeah for atName and atVersion we'll have to detect the running AT software. That will probably be specific to each AT application. Like for atName we can probably scan running processes for certain names. But atVersion will probably then be specific to each AT.
We should be able to do platformName now with os.platform()?
|
@mzgoddard @jugglinmike - I think this one is ready to review. I've updated the test suite to test the stuff I added (session.new) and a test path for receiving a voice message from the server. These tests do pass locally. |
gnarf
changed the title
There was a problem hiding this comment.
I'm not sure I checked everything. After comparing with https://w3c.github.io/at-driver/ this looks good to me.
There are some thing I'd like us to do but I don't think they need to be implemented in this change. (Like separating protocol logic and testing from websocket integration.)
| }); | ||
| }; | ||
|
|
||
| const onConnection = (websocket) => { | ||
| const send = (value) => websocket.send(JSON.stringify(value)); | ||
| const methodHandlers = { |
test/test.js
Outdated
| const {whenClosed} = await run([]); | ||
| return Promise.race([whenClosed, invert(connect(4382))]); | ||
| }); | ||
| // test('rejects unspecified protocol', async () => { |
There was a problem hiding this comment.
Should we use test.skip?
No description provided.