-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ipc wrappers #261
Conversation
c5b3fc3
to
7b129c0
Compare
@gmaclennan requesting review as in "I need help with figuring out the bugs" 😄 |
Co-authored-by: Andrew Chou <[email protected]>
test for this is currently failing. case with client.getProject is not behaving as expected
running into what seems like race condition in the second test related to |
src/ipc-wrapper/sub-channel.js
Outdated
/** | ||
* @extends {TypedEmitter<Events>} | ||
*/ | ||
export class SubChannel extends TypedEmitter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if i should avoid using TypedEmitter for this so that this can run in a browser environment as well (it's used for both server and client).
I havea local implementation that works using EventTarget based on this helpful post.
one of the main differences is the spec for the dispatchEvent()
method:
The dispatchEvent() method of the EventTarget sends an Event to the object, (synchronously) invoking the affected event listeners in the appropriate order. The normal event processing rules (including the capturing and optional bubbling phase) also apply to events dispatched manually with dispatchEvent().
Most notably the part about synchronously invoking listeners. Not sure if the Node implementation adheres to this or if it still behaves similar to an event emitter, but wondering if this specific detail could cause problems in our case or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess the simpler option would be using https://github.com/primus/eventemitter3, which is what rpc-reflector uses.
it's cool that the EventTarget approach works for our use case though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed via e067dc3 (using eventemitter3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this, this is tricky code and you've solved it really well. I realised reading through this that we need to handle the case of a project not being found, or an error when doing manager.getProject()
. I have given a suggestion about how to do this. We don't need a whole SubChannel
for this, we just need to pass a message somehow to the client.
src/ipc-wrapper/client.js
Outdated
* @param {import('../types.js').ProjectPublicId} projectPublicId | ||
* @returns {Promise<import('rpc-reflector/client.js').ClientApi<import('../mapeo-project.js').MapeoProject>>} | ||
*/ | ||
function createProjectClient(projectPublicId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this out of the scope of get()
, into the createMapeoClient()
scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed via fd58298
src/ipc-wrapper/sub-channel.js
Outdated
case 'active': { | ||
const { id, message } = value | ||
|
||
if (this.#id !== id) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but maybe better to put this above the switch statement (small edge case - if a subchannel was created and never started, it would accummulate a queue of all messages to all other subchannels).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed via 612f068
Closes #259
Adds IPC wrappers to allow usage where there are separate processes for the "client" and "server" side code such as React Native (via NodeJS Mobile) and Electron. These platforms typically provide some kind of channeling API that sets communication over an IPC layer, but plain usage of these APIs usually requires a decent amount of boilerplate code that matches client IPC requests to the desired server function calls and then communicating results back to the client. Using
rpc-reflector
, these wrappers bypass the boilerplate and allow normal usage of the server side API to occur on the client side.