Skip to content
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: docker compose for entire stack #20

Closed
wants to merge 4 commits into from

Conversation

beorn
Copy link
Contributor

@beorn beorn commented Apr 25, 2024

Provide a compose.yml for the entire stack:

  • one root image (yredis) and one image for each of the demo/* apps
  • the services inside of the demos/auth-express/server.js are run as separate microservices just to show them at the compose level (authperm, docupdated, and serving the actual demo)
  • depends on fix: package dependencies #19 and feat: graceful shutdown #18

Copy link
Member

@dmonad dmonad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a great addition!

I felt motivated to work on my own docker-compose file at the same time -.-

I don't feel attacked to my approach. However, I'd like to keep it as simple as possible. Can you please refactor and rebase?

Feel free to do everything in a single PR. I won't touch the docker files and the package*.json files for a few days anyway.

shared: &shared
env_file: .env
# overrides to connect services inside of Docker overlay network
environment: &shared-env
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea!

If the compose file overrides the configurations, then we only need to generate the auth tokens in .env

#
# demo apps
#
demo-codemirror: &demo-authexpress
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could keep the main compose file small. IMHO it would be better to move the compose file to the demos and inherit from the main compose file, like I'm currently doing.

@@ -0,0 +1,7 @@
FROM node

WORKDIR /app/demo/auth-express
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this to app/demo/auth-express

@beorn
Copy link
Contributor Author

beorn commented Apr 27, 2024

Ok, I'll resubmit as part of a new PR rebased on the latest master in a few days.

@beorn beorn closed this Apr 27, 2024
@beorn
Copy link
Contributor Author

beorn commented Apr 28, 2024

I had a few questions:

Core vs demo

How to see the separation between 'core' and 'demo' stuff - currently there's a ./bin/auth-server-example.js that is the same as the ./demos/auth-express/server.js. Should this all be in demo?

Packagizing?

Perhaps it would make sense to make a monorepo with each of the services as packages, and some of them being demos? E.g., (also trying to make names a bit longer/expressive - but I see it'd be nice to keep them short too):

workspaces: ["apps/*", "packages/*"]

# core, and perhaps published as npm packages:

  apps/yr-sync (server)
  apps/yr-compactor (worker)
  packages/yr-utils

# not core:

  apps/ex-yr-auth-server
  apps/ex-yr-backup-server
  apps/ex-demo-codemirror
  apps/ex-demo-blocksuite

The parts that need to be customizable in the core services could be made customizable through hooks/callbacks/configs.

APIs for storage

There seems to be several ways to provide storage for yjs docs, which is good but also a bit confusing:

  • y-provider types - plug straight into ydoc
  • AbstractStorage in server.js
  • *-level backends - upgrade y-leveldb to abstract-level and connect through abstract-level
  • presumably you can also add y-provider or AbstractStorage plugins for other "meta adators" such as keyv (KV) and knex (SQL)

It would perhaps be useful to map out what the generalized storage APIs need to be, and to provide an API to implement to - do we need one for connecting into ydocs directly and one for the functionality that y-redis requires?

@dmonad
Copy link
Member

dmonad commented Apr 28, 2024

The bin/auth-server-example.js and the rest of the project use uws for handling HTTP & websocket connections, which I chose for performance reasons. However, I probably wouldn't recommend it to "normal" users at this time, as there is basically no ecosystem around uws (for developing a rest backend, or static file server).

The ./demos/auth-express/server.js server is built using express, which has much more traction, but is definitely not the "best" technology IMHO.

I want to fill the demos repository with different implementations of the auth server that users can use to get started. The tests use bin/auth-server-example.js - which is why it is part of the "main project".

Perhaps it would make sense to make a monorepo with each of the services as packages, and some of them being demos? E.g., (also trying to make names a bit longer/expressive - but I see it'd be nice to keep them short too):

Maybe eventually. For now, this should be fine. I use the same structure in all my projects and I like to keep it simple. There are definitely different flavors of simple. At least this approach is consistent with my other projects.

It would perhaps be useful to map out what the generalized storage APIs need to be, and to provide an API to implement to - do we need one for connecting into ydocs directly and one for the functionality that y-redis requires?

The advantage of y-redis over other implementations is that it doesn't keep the Yjs document in-memory. Hence, you can't use the other providers in y-redis.

If you want to use y-redis, you should definitely implement AbstractStorage. It's pretty easy, but we are still missing documentation.

You can use any other persistence provider on the client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants