Use socket API #70
Use socket API #70
Conversation
|
Current blocking mm issue: jl777/SuperNET#659 |
|
These issues are also worth a read to get an idea of the socket functionality: |
app/renderer/marketmaker-socket.js
Outdated
|
|
||
| _parseData(data) { | ||
| return new Promise(resolve => { | ||
| const reader = new FileReader(); |
There was a problem hiding this comment.
Can you add a short comment here on why FileReader is being used? It's not clear to me.
There was a problem hiding this comment.
The WebSocket data we get is binary, I need to use FileReader to get a string from the blob and then parse it as JSON. I'll add some comments to explain this 👍.
Could alternatively use:
fetch(URL.createObjectURL(blob)).then(res => res.json());which makes it a (tiny) bit more clear what's going on but adds a huge amount of noise to the network tab in dev tools.
There was a problem hiding this comment.
Makes sense. I prefer the FileReader approach.
There was a problem hiding this comment.
Just found this module: https://www.npmjs.com/package/read-blob
Do you think a comment is still necessary if I'm using that descriptive function name?
(I'll still is that module either way)
There was a problem hiding this comment.
I think it's clear enough without the comment if you use that module.
There was a problem hiding this comment.
Yeah, it's much more readable now: 5f27a66
const json = await readBlob.text(event.data);
const data = JSON.parse(json);
app/renderer/marketmaker-socket.js
Outdated
| this._ws = new WebSocket(endpoint, ['pair.sp.nanomsg.org']); | ||
| this.connected = new Promise(resolve => this._ws.addEventListener('open', resolve)); | ||
|
|
||
| const ee = new Emittery(); |
There was a problem hiding this comment.
You can just do this as a class field:
_ee = new Emittery();You can only do this one though as it seems class fields cannot make use of previous class fields, at least with our Babel transform.
There was a problem hiding this comment.
Normally I would agree with you but I think in this case it seems more confusing doing this a class field as it's referenced inside the constructor. It also doesn't reduce code at all.
constructor(endpoint) {
const ee = new Emittery();
this._ee = ee;
this.on = ee.on.bind(ee);
this.off = ee.off.bind(ee);
this.once = ee.once.bind(ee);
}becomes
_ee = new Emittery();
constructor(endpoint) {
const ee = this._ee;
this.on = ee.on.bind(ee);
this.off = ee.off.bind(ee);
this.once = ee.once.bind(ee);
}There was a problem hiding this comment.
I was thinking from a Swift perspective again, where using this is optional.
app/renderer/marketmaker-socket.js
Outdated
|
|
||
| async _handleMessage(event) { | ||
| const data = await this._parseData(event.data); | ||
| const queueid = Number(data.result.queueid); |
There was a problem hiding this comment.
Why are you coercing this to a number? I thought the returned queueid is already a number.
There was a problem hiding this comment.
I didn't trust the responses to always be a number. However, we're only doing a size comparison so it shouldn't matter if it's a string anyway, I'll remove it.
There was a problem hiding this comment.
If that's the case, I would rather throw a TypeError if it's not a number, so it doesn't silently pass.
app/renderer/marketmaker-socket.js
Outdated
| this.off = ee.off.bind(ee); | ||
| this.once = ee.once.bind(ee); | ||
|
|
||
| this._ws.addEventListener('message', this._handleMessage.bind(this)); |
There was a problem hiding this comment.
If you make _handleMessage a class field, you don't have to bind it.
app/renderer/api.js
Outdated
| if (!(endpoint && seedPhrase)) { | ||
| throw new Error('The `endpoint` and `seedPhrase` options are required'); | ||
| } | ||
|
|
||
| this.endpoint = endpoint; | ||
| this.token = sha256().update(seedPhrase).digest('hex'); | ||
| this.socket = false; | ||
| this.currentQueueid = 0; |
There was a problem hiding this comment.
I know the returned queueid is lowercase, but let's use camelCase ourselves => currentQueueId
|
@sindresorhus Are you able to re-review this? I've implemented the changes you requested and resolved all merge requests. I've also worked through the mm issues that were blocking this with James and they are now resolved. |
|
Looks good :) |
This uses the socket API by default for all commands.
This isn't a particularly neat solution but it was all James could implement last minute.
First we have to ping
getendpointwhich will start the ws server. Then we connect to it and listed to all messages, currently it sends us all P2P traffic.If we send an HTTP command with a non zero
queueidthen it will be queued internally by mm. mm will then return the command response along with it'squeueidover the ws when it's been processed.The way this is implemented is completely transparent, functionality should be exactly the same in ws mode or HTTP mode, all the matching up of HTTP responses or socket responses is abstracted away.
Just opening this PR for your feedback on this implementation. It's not ready to be merged yet, there are still bugs in mm that are stopping it from working.