-
Notifications
You must be signed in to change notification settings - Fork 149
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
example (sidecar): NodeJS sidecar example #772
Conversation
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.
It fails for me at the step of starting the sidecar. I was following the steps in the clients/node/README.md
❯ yarn build
yarn run v1.22.19
$ tsc
src/sidecar.ts:5:10 - error TS2305: Module '"electric-sql/client/model"' has no exported member 'ShapeManager'.
5 import { ShapeManager } from 'electric-sql/client/model'
~~~~~~~~~~~~
src/sidecar.ts:7:57 - error TS2307: Cannot find module 'electric-sql/cli/migrations' or its corresponding type declarations.
7 import { fetchMigrations, loadMigrationsMetaData } from 'electric-sql/cli/migrations'
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/sidecar.ts:88:11 - error TS7006: Parameter 'm' implicitly has an 'any' type.
88 m => m.ops.forEach(
~
src/sidecar.ts:89:13 - error TS7006: Parameter 'op' implicitly has an 'any' type.
89 op => {
~~
Found 4 errors in the same file, starting at: src/sidecar.ts:5
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
</picture> | ||
</a> | ||
|
||
# ElectricSQL - NodeJS sidecar |
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.
We need top-level README.md explaining the purpose of the example project.
I would maybe put the instructions that you have on the client at top level, because it covers starting the app and the sidecar.
```sh | ||
git clone https://github.com/electric-sql/electric | ||
cd electric/examples/node-sidecar/clients/node | ||
yarn backend:start |
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.
yarn
for installing deps
Then, start the sidecar: | ||
```sh | ||
cd electric/examples/node-sidecar/sidecar | ||
yarn start <path-to-db-file> # e.g. yarn start ../clients/node/electric.db |
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.
Would be nice to have a command to start the sidecar, but let's not do it now
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.
Yep, yarn start
is the command :-) you just have to specify the db file to use, that's it
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.
without having to cd to another folder
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'm not particularly a fan of putting a single top level command that runs everything because it blurs away how it really works and the fact that the sidecar and the application are 2 different programs. In fact, i think that we want to make that explicit to the user such that they understand that they first need to run the sidecar and then their application that connects to the sidecar. We can discuss this :-)
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.
Please double-check that all of the now exposed method have no side effects over the client state. If so, I think we can merge the changes to satellite and release a patch.
The instructions must be simple for the sidecar and right now they aren't. I have a bit of a strong opinion that the sidecar should start along the app that uses it. I'm open to discussion if you think otherwise, but I think that is the ideal experience for someone playing with this.
Note that we can't use pnpm, our scripts are not compatible with windows. I don't we need that once the patch is released.
* Polls Electric's migration endpoint for new migrations. | ||
* If there are new electrified tables we will sync them. | ||
*/ | ||
async pollMigrations(): Promise<void> { |
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.
This is okay for this shareable prototype, but for merging this couldn't we set a rawLiveQuery that watches when the migration version changes instead of polling?
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.
The idea is to only poll the server when we know there are changes.
/** | ||
* Notifies clients of actual data changes. | ||
*/ | ||
notifyDataChanged(): Promise<void> |
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.
Leave a TODO here that there are more events missing. e.g. connectivity state change
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 think client and server interfaces should live on a common location
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.
How would you do that? The server IPC interface must be provided by the sidecar, the client IPC interface must be provided by the application. Those are two separate packages. The only way i can envision this is by moving the client and server IPC interfaces to a package of its own that would then be used by the sidecar and the application. So then we would have:
- a package for the client and server IPC interfaces and their implementations
- the sidecar package that uses the server-side components of the IPC package
- the application that uses the client-side components of the IPC package
Let me know if this is what you want. We will need to think however how we will achieve this. Are we going to publish this IPC package to npmjs? Then it would probably also make sense to publish the sidecar.
Another possibility would be to define the IPC client and server interfaces and their implementations in the sidecar package and have the NodeJS client application use the sidecar as a dependency. Again, we need to think how we will make the sidecar available to those apps, it will probably need to be published.
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 think client and server interfaces should live on a common location.
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 think this organisation is conflating two concepts: the client of the sidecar and the code of the app. I think what we're calling clients/node
is in fact an example that provides a client socket implementation and an example app. I would extract the application code to a separate folder (keeping the structure of different languages).
This is a bit of personal taste so I won't fight much about it. What I really really want is the following:
The developer runs a single yarn start
command that runs a single file that starts the sidecar and the app. Users could manage the two things separately if they want, but I want to convey the pattern of starting the two things together. In other languages people would use shell or something to start the sidecar as a child process of the app.
Passing a port should be optional. In our case, since we're initialise the sidecar, we can get a random port from the OS and pass it to the app on the spot. Minimal static things.
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.
About conflating the two: every application that wants to use the sidecar needs to be a client of the sidecar (i.e. use some IPC mechanism). I've put the IPC client inside the app. Do you want to make the IPC client a separate nodejs package that nodejs applications can use?
As i said in another comment, i'm not sure we want to provide a single start command for 2 reasons:
- Users need to understand that the example consists of 2 parts: the sidecar and the application that connects to it. So imo it's better to make that explicit by having to run them separately.
- Providing a single start command makes it more difficult for the user to run the sidecar example with several instances of the client application.
Whereas, with 2 separates commands it is immediately clear that they just need to run the sidecar once, and then as many application instances as they want.
Regarding the random port, sure we can do this but we're just increasing the complexity of the example a bit more. The minimal thing would be to make the port configurable.
@@ -23,7 +23,7 @@ export class WebSocketNode extends EventEmitter implements Socket { | |||
|
|||
this.socket.on('open', () => this.emit('open')) | |||
this.socket.on('message', (data) => this.emit('message', data)) | |||
this.socket.on('error', (_unusedError) => | |||
this.socket.on('error', (_unusedError) => |
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.
you don't have to commit this file
Not sure. You only want 1 sidecar and potentially several applications. So if you run 2 applications you don't want the sidecar to be started twice. But we can discuss this :-) |
I’m considering all comments have been addressed during our call. I’ll get back on the next push |
13eec85
to
c43259d
Compare
… top-level readme
@balegas I applied the changes we discussed and introduced a configuration file as originally proposed by @samwillis. Note the top-level readme and the top-level The sidecar still requires a few changes to the ts-client. Therefore, you will need to There are 2 subtleties still:
|
8638018
to
1ad9613
Compare
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 with making sidecar work with only public interfaces.
- Thanks for removing the infra code from run.js
- README.md should now have the instructions to launch the infrastructure for the readme to be self contained (note that we also need to cd into folders to install deps)
I think after that it's all done. I need to be able to run the project just by following instructions in top-level README.md
Updated readme with instructions for running the infrastructure. |
@samwillis may want to have a look at how the sidecar and the example application are built. |
@kevin-dp, all looks good to me, vanilla If you wanted to, the top level run.js could be an ESM with a |
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 quite felt the tension of running every single command in Windows, but everything worked!
- I added a few more details to the README to incentivize exploration.
- I think the commands for running the backend should live on the sidecar side, because things are tied to the JS ecosystem and people on other envs, would not necessarily have the package.json commands, or would have to port them.... there is maybe things to improve here, but I'll not obsess with that. I think the instructions are good and we can discuss with people
- We should raise the attention of people from the community to this example and incentivise them to contribute
It's done and looks great. Thank you
aa57b7e
to
b31d2fd
Compare
This PR introduces a NodeJS sidecar and an example application using the sidecar and the DB directly.
Steps to test the sidecar:
First build and pack the ts-client:
cd clients/typescript pnpm build npm pack
Modify the package.json of the sidecar (cd sidecar) to use the packed ts-client. The application can use the published ts-client.
First start the backend and apply your migrations:
cd clients/node yarn backend:start yarn db:migrate
Then run the sidecar:
cd sidecar yarn start ../clients/node/foo.db
This will create the
foo.db
db file in theclients/node
folder.Then run the client application(s):
cd clients/node yarn start foo.db