RPC reflector #3

tomasciccola · 2023-07-06T17:28:36Z

This branch is a good starting point for a mapeo-mobile with rpc-reflector integrated.
Thinks to take into account:

Initially there were two different wrappers (frontend and backend) for the channel thats used for bridging. I basically merged them into one file trying to accommodate both cases. This now lives on src/shared/lib/message-port-like.js
For shared files between the frontend and backend I added a src/shared folder. I don't if this is a good approach or if there's a best practice.
The api (now called MapeoClient) lives on src/shared. Its a plain object but probably I'll turn it into a class when adding the mocked api stuff
I modified the scripts/build_backend.sh file so to copy the shared files in a way that preserves the relative file path and not mess imports. I don't know if this is a good approach, but it works

gmaclennan

I few bits of feedback. I think it's ok to leave the useNodejsMobile() hook here for this PR, because it will work ok, but we should address that in a later PR.

Also to address in a later PR: #4

gmaclennan · 2023-07-06T17:42:05Z

metro.config.js

+    extraNodeModules: {
+      stream: require.resolve('readable-stream'),
+    }


This shouldn't be necessary with the latest rpc-reflector

gmaclennan · 2023-07-06T17:42:40Z

package.json

@@ -10,9 +10,14 @@
    "lint": "eslint . --ext .js,.jsx,.ts,.tsx"
  },
  "dependencies": {
+    "assert": "^2.0.0",


This also should not be necessary with latest rpc-reflector

It seems that removing this breaks the app, don't know the real reason though...

gmaclennan · 2023-07-06T17:42:58Z

package.json

    "nodejs-mobile-react-native": "^0.8.2",
    "react": "18.1.0",
-    "react-native": "0.70.6"
+    "react-native": "0.70.6",
+    "readable-stream": "^4.4.1",


You can remove this too

Same with this, I had to leave the readable-stream dep for the app to work. But the metro translation to stream wasn't necessary. react-native mysteries I guess...

gmaclennan · 2023-07-06T17:45:08Z

scripts/build-backend.sh

@@ -27,6 +27,8 @@ else
  echo "Set Build Native Modules on"
 fi
 cp -r ./src/backend ./nodejs-assets
+mkdir -p ./nodejs-assets/shared/lib
+cp -r ./src/shared/* ./nodejs-assets/shared/


Does this actually work in the built app? I thought the nodejs-mobile project only bundles code in the nodejs-assets/nodejs-project folder

yeah, this build correctly. It seems that the app can see up the nodejs-project/ directory. But probably @achou11 may see shortcomings of this?

gmaclennan · 2023-07-07T11:23:02Z

src/frontend/App.tsx

+  const [clientApi, setClientApi] = React.useState<typeof MapeoClient>();
  React.useEffect(() => {
    nodejs.start('loader.js');
-
-    const subscription = nodejs.channel.addListener('message', msg => {
-      console.log('RECEIVED MESSAGE', msg);
-    });
-
+    const channel = new MessagePortLike(nodejs.channel);
+    setClientApi(createClient<typeof MapeoClient>(channel));
    return () => {
-      // @ts-expect-error
-      subscription.remove();
+      channel.close();
    };
  }, []);
-
-  return {
-    send: (msg: string) => nodejs.channel.send(msg),
-  };
+  return clientApi;


nodejs.start() shouldn't be in here - that should just happen once when the JS file first loads, not tied to the react lifecycle.

I'm not sure if the channel and clientApi need to be in here either, nor tied to the useEffect. This isn't a side-effect, and there is no async here, so this code is rendering the app once with no clientApi, then calling setClientApi() and rendering again. If we want to do a useClientApi() it should probably just be a wrapper for useMemo(), since we only want it once, or we could just put the client api in the global scope.

ah yeah this stuff I worked on - not sure why I did this but fully agree that it should be set up outside of react

Yeah, this is basically refactoring what was already there, probably this will be worked on here

ah remember why i had it set up this way - mapeo mobile does it similarly (where api.startServer calls nodejs.start()):

https://github.com/digidem/mapeo-mobile/blob/ca167c257b90ba74b0e63e623f405aed98d86145/src/frontend/AppLoading.tsx#L77-L81

either way, agree it probably could just be moved outside for the sake of this project

addressed in #7

achou11

Noticed some issues as I explored these changes locally:

rpc-reflector was not being specified as a dep for the backend project. It was only being specified for the top-level. Guessing that this causes some of the issues related to certain deps seeming to be needed but not really
Needed to polyfill process, util, and events in order for rpc-reflector to not complain on the React Native side. Using something like https://github.com/ionic-team/rollup-plugin-node-polyfills to achieve and updating the metro config worked for now
The MessagePortLike class needs to add an off method, which is what rpc-reflector uses to determine if it's a viable message port interface: https://github.com/digidem/rpc-reflector/blob/9bcc507edd6e498145593bf491a927e3ab5ba652/lib/is-message-port-like.js#L19-L21
The MessagePortLike doesn't fully account for the differences between the nodejs mobile channel on React Native vs NodeJS Mobile. On NodeJS Mobile, it extends the EventEmitter, so that's fine. On React Native, it extends a custom implementation of an EventEmitter, which behaves differently from Node's EventEmitter
Not as important, but currently this PR introduces deps changes via npm 6 (previously was 8). Ideally we'd keep it at 8 but Tomas mentioned potential issues getting 8 to work in his environment, so may need some looking into

ErikSin · 2023-07-21T21:44:09Z

@tomasciccola I scheduled a meeting with you for tuesday to talk about these things:

I am unable to build this app on my phone/emulator
Typescript typing is not working for me.
Review general architecture
Can we talk about strategies of creating fake data (ts object, json objects....etc)

ErikSin · 2023-07-25T22:08:25Z

As discussed in out meeting today, this is the list of todos for the mock API for observations

To do (Backend)

Generate observations (as JSON) using 'Mapeo-Mock-Data'.
Convert JSON into objects, and creating them as files in `src/utils'
On creation of Mapeo-Core, write them objects to database

To do (Front-end)

Ask Gregor/Andrew about typings
Move createClient()outside of react lifecycle/hook
Create useObservation and useObservations hook
Bring Screens from mapeo-mobile which utilize observations

tomasciccola · 2023-07-26T14:52:11Z

So, the type of mapeoClient.observation is now just a plain object, this was so that the generated docs from mapeo-mock-data would be valid observations. In the, not so distant, future we are going to get the type from mapeo-core-next so I didn't took the time to get more strict typing.
Additionally I created src/utils/mockData.js as javascript (not typescript) despite what initially discussed with Erik, this is 'cause the backend, which is plain js, is loading this file and transpiling didn't make much sense.
I know @ErikSin we talked about using ts for this file, but in the future the JSDoc on the mockData file won't be much more than

/* @type {import('mapeo-core-next').Observation[]} */
const mockData = [...]

tomasciccola · 2023-07-27T15:28:41Z

So, with @ErikSin we went back and forth with setting clientApi as a function initialized on App.tsx or as a hook. For now its a hook, but I like the approach that @gmaclennan proposed, copying and pasting from the chat:

export function WithClientApi({ children, client }) {
  return (
    <ClientApiContext.Provider value={client}>
      {children}
    </ClientApiContext.Provider>
  )
}

// this will probably be smth like initClient() that sets up the MessagePort like and coordinates server state
const client = createClient(port) 

<WithClientApi client={client}>
  <App />
</WithClientApi>

// That way if you want to unit-test components then you can do
const client = {
  observation: {
    getMany(): [...mockedData]
  }
}

<WithClientApi client={client}>
  <ComponentToUnitTest />
</WithClientApi>

So basically use a react context. We can set up that in this PR, or merge it in the state that's in, and do it later

gmaclennan · 2023-08-15T10:05:12Z

src/backend/index.js

-const log = debug('mapeo-mobile-node-next');
+// @ts-expect-error
+const channel = new MessagePortLike(rn_bridge.channel);
+channel.start();


Can you move this (.start()) to after the RPC server is created? Strictly this will be ok as-is because everything happens in the same tick, but if someone changes that, e.g. with an await before createServer(), then there would be a chance that messages would be lost.

gmaclennan · 2023-08-15T10:11:47Z

src/backend/message-port-like.js

+  /**
+   * @param {Channel} channel
+   */
+  constructor(channel) {


Since this is strictly tied to the static import nodejs-mobile-react-native.Channel, I think this should just be an import in this file, and not a constructor parameter, so this file encapsulates the Channel dependency.

gmaclennan · 2023-08-15T10:11:58Z

src/frontend/lib/MessagePortLike.ts

+  #state: ServerState = 'idle';
+  #queuedMessages: any[] = [];
+
+  constructor(channel: Channel) {


Since this is strictly tied to the static import nodejs-mobile-react-native.Channel, I think this should just be an import in this file, and not a constructor parameter, so this file encapsulates the Channel dependency.

gmaclennan · 2023-08-15T10:16:07Z

src/frontend/lib/MessagePortLike.ts

+  on(event: string, listener: (...args: any[]) => any) {
+    const sub = this.addListener(event, listener);
+
+    const registry = this.#eventsSubscriptions.get(event);
+
+    if (!registry) {
+      this.#eventsSubscriptions.set(event, new WeakMap([[listener, sub]]));
+      return this;
+    }
+
+    if (!registry.has(listener)) {
+      registry.set(listener, sub);
+    }
+
+    return this;
+  }
+
+  off(event: string, listener: (...args: any[]) => void) {
+    return this.removeListener(event, listener);
+  }
+
+  removeListener(event: string, listener: (...args: any[]) => void) {
+    const registry = this.#eventsSubscriptions.get(event);
+
+    if (!registry) {
+      return;
+    }
+
+    const subscription = registry.get(listener);
+
+    if (subscription) {
+      subscription.remove();
+      registry.delete(listener);
+
+      // TODO: Call this.#subscriptions.delete(event) if no more listeners?
+    }
+
+    return this;
+  }
+
+  removeAllListeners() {
+    this.#eventsSubscriptions.clear();
+    this.removeAllListeners();
+  }


This seems to be re-implementing an event emitter, which is a hard thing to do and is full of edge cases. Why not use an existing library and extend from that rather than the react-native eventemitter?

think the context for this (as well as the other review comments) mostly lives with stuff that I updated via #8 (see PR description)

but yeah if there's something we can use to simplify this, probably a better approach

think the main trickiness was that since this is tied to nodejs mobile's Channel implementation (which extends React Native's EventEmitter), it doesn't have the expected interface as the traditional Node EventEmitter, which RPC reflector relies on.

React Native's EventEmitter uses a subscription based API i.e. addListener returns a Subscription and there are no methods like on and off, hence why this class is reimplementing some of those while also tying it with the Nodejs mobile Channel

link to React Native EventEmiiter implementation that the Channel extends (for the version of React Native we're using):

https://github.com/facebook/react-native/blob/v0.70.6/Libraries/vendor/emitter/EventEmitter.js

note that it's missing methods that rpc-reflector checks to determine message port compat, hence this wrapper class to begin with and the reimplementation of some event emitter like methods:

https://github.com/digidem/rpc-reflector/blob/main/lib/is-message-port-like.js#L19-L21

Yes, but why extend MessagePortLike from the react-native EventEmitter? Why not extend EventEmitter from events or EventEmitter3 - those have the methods that rpc-reflector checks. I would have thought that would work fine? There is no relationship between the type of EventEmitter that Channel extends and what MessagePortLike needs to be, except how you internally handle attaching and removing listeners from the channel.

yeah, because what you're doing here is essentially reimplementing an EventEmitter, and also creating a compatibility layer with the react native EventEmitter, which is a maintenance overhead, and probably needs a lot more work to catch all the edge cases that the npm libs already check for.

after clearing up my misunderstandings with Gregor elsewhere, the actionable changes here would be:

use one of the aforementioned libraries to extend for this class, instead of React Native's EventEmitter

Remove the custom implementations of on, off, removeListener, removeAllListeners.

Remove this.#eventSubscriptions field

Remove line 41 (channel.addListener('message'))

Does extending from tiny-typed-emitter would work? (I'm mentioning it 'cause the backend/message-port-like.js extends from that...

Don't think so because it relies on Node's EventEmitter, which isn't available in React Native's runtime

tiny-typed-emitter is just require('events').EventEmitter. It works in node, but you need to have a polyfill for react native. You can just use events which is handy because it has the same name - you just need to install it from npm in the frontend - or you can use EventEmitter3 with tiny-typed-emitter if you configure Metro to package events -> EventEmitter3

achou11 · 2023-08-15T14:40:33Z

Having trouble getting this to work when running locally. Just to check, is it working for other people?

tomasciccola · 2023-08-15T17:00:17Z

Having trouble getting this to work when running locally. Just to check, is it working for other people?

Yep, up until now, when I implemented the changes proposed by @gmaclennan, now the app crashes upon opening it, and logcat doesn't give any useful output. Gonna rollback the chances and introduce them one by one to see where it fails...

that don't make sense to work on right now

Buffer

I don't need to change de buildBackend script

Also, moved the starting of the port outside of the message-port-like classes on both sides, for the sake of symmetry

handle case in which the server sended the message before the listener is attached

tomasciccola requested review from gmaclennan, achou11 and ErikSin July 6, 2023 17:28

gmaclennan reviewed Jul 7, 2023

View reviewed changes

gmaclennan mentioned this pull request Jul 7, 2023

Create minimal mocked Mapeo API and build hooks for client integration #5

Closed

4 tasks

achou11 mentioned this pull request Jul 10, 2023

simplify nodejs mobile initialization from client #7

Closed

achou11 reviewed Jul 10, 2023

View reviewed changes

achou11 mentioned this pull request Jul 12, 2023

some adjustments to rpc reflector integration #8

Merged

gmaclennan changed the title ~~Rcp reflector~~ RPC reflector Aug 15, 2023

gmaclennan requested changes Aug 15, 2023

View reviewed changes

achou11 mentioned this pull request Aug 15, 2023

more proposed changes to the rpc-reflector integration #22

Merged

Tomás Ciccola and others added 13 commits August 24, 2023 09:41

feat: initial integration of rpc-reflector

062a487

chore: inline api in src/backend to avoid bundling issues

20d6504

that don't make sense to work on right now

chore: make rpc call as a promise to catch errors

1bef25b

chore: get some info about backend

fce2104

fix: get this working

a741fed

feat: rpc-reflector is working. Move api to src/backend/api.js

905f0f7

feat: add patch-package to postinstall and patch rpc-reflector to find

f18fdbf

Buffer

feat: shared message-port-like.js between backend and frontend...

f3c78cc

feat: call channel.start() inside Class, handle removeListener case

14ea062

feat: use main rpc-reflector, put api on shared dir

b860544

chore: remove readable-stream resolution on metro.config.js

90dff36

package installation fixes for rpc-reflector

415c952

add polyfills needed for rpc-reflector on client

7386cb1

achou11 and others added 19 commits August 24, 2023 09:44

remove node polyfills

d80b9be

fix type usage

2bafdef

separate implementations for MessagePortLike

7a807a4

update rpc-reflector dep in backend

2c1456d

feat: useClientApi hook

08397fc

feat: start integration of mapeo-core class for mock api

b02042f

feat: create mock data so that getMany returns something

0906cf0

feat: clientApi is on src/frontend/lib and is not a hook anymore

0050633

added src/utils/mockData.ts

15dedb6

mockData.ts -> mockData.js

0ea8271

move mockData inside backend so that

2294576

I don't need to change de buildBackend script

feat: query the server for its state, and only then start the port

81c5920

Also, moved the starting of the port outside of the message-port-like classes on both sides, for the sake of symmetry

feat: querying server state

6358e60

handle case in which the server sended the message before the listener is attached

feat: turn clientApi into a hook

96aebfb

update deps

05259bd

update frontend MessagePortLike implementation

ab06042

update backend implementation to be more structured

36ce2cd

use api singleton approach instead of useClientApi

1e4e19c

add state update in backend initialization

c6a8ad2

ErikSin force-pushed the rcp-reflector branch from 8a11839 to c6a8ad2 Compare August 24, 2023 16:50

chore: update package-lock

d0373dd

ErikSin merged commit 6d9e242 into main Aug 24, 2023

CDFN mentioned this pull request May 14, 2024

feat: display photos in observations #344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC reflector #3

RPC reflector #3

tomasciccola commented Jul 6, 2023

gmaclennan left a comment

gmaclennan Jul 6, 2023

gmaclennan Jul 6, 2023

tomasciccola Jul 10, 2023

gmaclennan Jul 6, 2023

tomasciccola Jul 10, 2023

gmaclennan Jul 6, 2023

tomasciccola Jul 10, 2023

gmaclennan Jul 7, 2023

achou11 Jul 7, 2023

tomasciccola Jul 10, 2023

achou11 Jul 10, 2023

achou11 Jul 10, 2023

achou11 left a comment

ErikSin commented Jul 21, 2023

ErikSin commented Jul 25, 2023 •

edited

Loading

tomasciccola commented Jul 26, 2023 •

edited

Loading

tomasciccola commented Jul 27, 2023

gmaclennan Aug 15, 2023

gmaclennan Aug 15, 2023

gmaclennan Aug 15, 2023

gmaclennan Aug 15, 2023

achou11 Aug 15, 2023

achou11 Aug 15, 2023 •

edited

Loading

achou11 Aug 15, 2023 •

edited

Loading

gmaclennan Aug 15, 2023

gmaclennan Aug 15, 2023

achou11 Aug 15, 2023 •

edited

Loading

tomasciccola Aug 15, 2023

achou11 Aug 15, 2023

gmaclennan Aug 15, 2023

achou11 commented Aug 15, 2023

tomasciccola commented Aug 15, 2023

RPC reflector #3

RPC reflector #3

Conversation

tomasciccola commented Jul 6, 2023

gmaclennan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 left a comment

Choose a reason for hiding this comment

ErikSin commented Jul 21, 2023

ErikSin commented Jul 25, 2023 • edited Loading

To do (Backend)

To do (Front-end)

tomasciccola commented Jul 26, 2023 • edited Loading

tomasciccola commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

achou11 Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 commented Aug 15, 2023

tomasciccola commented Aug 15, 2023

ErikSin commented Jul 25, 2023 •

edited

Loading

tomasciccola commented Jul 26, 2023 •

edited

Loading

achou11 Aug 15, 2023 •

edited

Loading

achou11 Aug 15, 2023 •

edited

Loading

achou11 Aug 15, 2023 •

edited

Loading