1. Remove Encryption, Refactor Client #156
Conversation
…t in a more centralised manner.
…Client sub-parts.
timoxley
force-pushed
the
remove-encryption-code
branch
from
cfc2b5c to
197232e
Compare
…SAGE_TYPES to CONTENT_TYPES.
…stent with bad url parsing errors.
…SAGE_TYPES to CONTENT_TYPES.
…rrors consistent.
harbu
left a comment
harbu
left a comment
There was a problem hiding this comment.
LGTM; Skimmed through since it was a pretty big PR.
A few very minor comments. Prolly were in the code before these changes so feel free to ignore.
One thing that I took note of with the current code base: lots of state handling, registering / unregistering, bookkeeping, connection management etc. Wonder how much of this is really necessary and how much a result of accidental complexity.
src/Publisher.js
Outdated
| client.on('connected', this.onConnected) | ||
| } | ||
|
|
||
| async onConnected() { |
There was a problem hiding this comment.
No await keyword in method. Is async needed?
src/Publisher.js
Outdated
| // Check pending publish requests | ||
| const publishQueueCopy = this.publishQueue.slice(0) | ||
| this.publishQueue = [] | ||
| publishQueueCopy.forEach((publishFn) => publishFn()) |
There was a problem hiding this comment.
I wonder if it would be better to remove an item from the queue only after is has been successfully published?
There was a problem hiding this comment.
Yeah that's probably better, though this is the old-code as-is.
I have a 'queue' in the new version but it isn't clamped at a max value like this, just works on promises. Should probably get a proper queue again in order to limit the max outstanding.
| stream.getSubscription(sub.id).handleResending(response) | ||
| } | ||
|
|
||
| async onUnicastMessage(msg) { |
There was a problem hiding this comment.
right, doesn't help for event handlers
| this._checkAutoDisconnect() | ||
| } | ||
|
|
||
| async onClientConnected() { |
src/Subscriber.js
Outdated
| }) | ||
| }) | ||
| } catch (err) { | ||
| this.client.emit('error', err) |
There was a problem hiding this comment.
Since _resendAndSubscribe has a catch above, seems like anything that would fall here would be a "genuine" error worth throwing?
There was a problem hiding this comment.
yep correct, good spotting
timoxley
changed the title
* Promisify subscribe/unsubscribe. * Fail _request call if unsubscribe/subscribe request fails to send. * Fix missing import in resend test. * Fix browser tests. * Clean up unused function in test. * 4. Refactor Publish (#164) * Refactor & co-locate publish code. * Avoid using expensive ethers.Wallet.createRandom() calls in test. e.g. 1000x calls with ethers: 14s, randomBytes: 3.5ms. * Ensure messageCreationUtil is cleaned up after test. * Fix non-functional MessageCreationUtil test. * Swap out receptacle for more flexible mem/p-memoize/quick-lru. * Convert LoginEndpoints test to async/await. * Remove calls to ensureConnected/ensureDisconnected in test. * 5. Message Sequencing – Guarantee sequence follows publish order & Prevent backdated messages silently breaking future publishes (#166) * Improve authFetch logging. * Update message sequencer to strictly enforce message order. * Queue publishes per-stream otherwise can skip forward then back even when messages published in correct sequence. * Add partial solution to broken backdated messages, at least doesn't break regular sequential publishes. * Tidy up, add some comments. * Move publish queue fn into utils.
2. Refactor Connection & Retry Handling
|
This all lives in the |
Removed encryption logic in preparation for re-adding the encryption/decryption logic in a more "pipeline"-like architecture.
Did some substantial refactoring of the client to try isolate related pieces of logic & state: Connection, Publishing, Subscribing + Resends. Flattened some logic and moved event handlers into methods so it's possible to actually remove them, but otherwise the code is largely unchanged, things are just moved around a bit.
Followed by: