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

test(yjs): Spike #84

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

JannikStreek
Copy link
Collaborator

@JannikStreek JannikStreek commented Apr 2, 2024

closes #68

This is not to be considered production ready code or anything to merge. This is supposed to be an example on how yjs works.

Learnings:

  • Integrating y-websocket into the existing next.js server seems to be cumbersome, at least I struggled with a simple problem: the rooms are expected to have a url like /socket/ROOM_A. Unfortunately next.js uses its own logic and is searching for a module with the same name (ROOM_A) for routing...didn't figure out how to deactivate that. Maybe rewrites help.
    • At least for http requests its: /api/room/[id]/route.ts - currently not working for web sockets via next-ws.
  • y-websocket updates do not forward the whole state on each update. So it's not as easy as just storing updates each time in the database. We would need to write an algorithm (can be as simple as saving every x minutes the whole state) when to only forward diffs to other users and when to save the whole document.
  • So I tried y-redis which is now integrated in this PR. It does two things: 1) serves as y-websocket backend, 2) caches the smaller diff updates in redis and then later stores the whole document in Postgres. This is configurable. So takes care of the whole process. The y-redis server is, however, too complicated for us as its also implements an auth layer before entering a room via JWTs. I don't think we need that, so we will need to throw all of this out. For this spike, I hacked my way around it.
  • I have used quill editor with the existing bindings for yjs. All easy to integrate. Sure you can write your own editor and binding, but this one is does it all for no work at all...guess its also possible to extend it.
Bildschirm­foto 2024-04-02 um 20 43 28

Instructions

You need redis, Postgres and the branch => Better use my docker compose setup.

docker compose up -d
docker compose exec app npm run dev

EDIT: and please update the .env file, some new additions there.

@JannikStreek
Copy link
Collaborator Author

@SamTV12345 @AugustinMauroy feel free to test

docker-compose.yml Outdated Show resolved Hide resolved
Co-authored-by: Augustin Mauroy <[email protected]>
@AugustinMauroy
Copy link
Contributor

AugustinMauroy commented Apr 2, 2024

It's possible to bind the editor with our custom tools bar ? Or we should have our custom style on the editor

@JannikStreek
Copy link
Collaborator Author

It's possible to bind the editor with our custom tools bar ?

you have to write your own binding. yjs comes with bindings for the popular editors, if you want to build a custom one, you need your own bindings.

@AugustinMauroy
Copy link
Contributor

you have to write your own binding. yjs comes with bindings for the popular editors, if you want to build a custom one, you need your own bindings.

Humm very interesting. I would have liked to see our publisher. But you have to look at the development/maintenance costs compared to the flexibility it brings. Also, if we made our own, the security holes would be with us.

@AugustinMauroy
Copy link
Contributor

AugustinMauroy commented Apr 2, 2024

Actually some people just answered my question:/api/rooms/[id].js would be the way to go, haven't tried it yet with the next-ws stuff.

I didn't 100% catch you issue but. if you do something like that /api/rooms/[id]/route.ts next will compile

next docs about route handler

@JannikStreek
Copy link
Collaborator Author

Actually some people just answered my question:/api/rooms/[id].js would be the way to go, haven't tried it yet with the next-ws stuff.

I didn't 100% catch you issue but. if you do something like that /api/rooms/[id]/route.ts next will compile

yes, would also be possible. would leave the logic when to store the whole document and when the diff to us. We could choose if we want to go the redis or the custom backend path...both possible I guess. redis is more complicated as setup but the algorithm is already there.

@AugustinMauroy
Copy link
Contributor

yes, would also be possible. would leave the logic when to store the whole document and when the diff to us. We could choose if we want to go the redis or the custom backend path...both possible I guess. redis is more complicated as setup but the algorithm is already there.

It's good that you said it again. But if the instance doesn't use docker (for example a simple VPS) this will add complexity to the setup. How is this currently done? And again, what is the maintenance cost? Because a custom algo isn't death.
Also, from a performance point of view, you have to look at whether you're running a full redis or just running a bit of JS when you need it (normally nextjs handles this well).

@AugustinMauroy AugustinMauroy added enhancement New feature or request infrastructure Pull requests concerning infrastructure labels Apr 2, 2024
@JannikStreek
Copy link
Collaborator Author

JannikStreek commented Apr 2, 2024

yes, would also be possible. would leave the logic when to store the whole document and when the diff to us. We could choose if we want to go the redis or the custom backend path...both possible I guess. redis is more complicated as setup but the algorithm is already there.

It's good that you said it again. But if the instance doesn't use docker (for example a simple VPS) this will add complexity to the setup. How is this currently done? And again, what is the maintenance cost? Because a custom algo isn't death. Also, from a performance point of view, you have to look at whether you're running a full redis or just running a bit of JS when you need it (normally nextjs handles this well).

there are no perfect solutions, just tradeoffs. Yes, on a simple vps it may be difficult, but most are also offering databases and redis. And even without, it's still possible. We will need to implement the web socket backend part ourselves which is currently handled by y-redis. As a reminder: the scope of this PR was not to offer a perfect solution. the scope of this pr was to get a feeling for yjs and see it in action. I am happy to try out another way of implementing yjs, but there is no silver bullet here.

Because a custom algo isn't death.

I strongly disagree here, even if it's not death. Merging conflicting updates in a reliable way is complex, time consuming to code, prone to errors and results in higher code complexity. And this is completely solved within yjs even with all the editor bindings. Why reinvent the wheel here if you get it for free?

@AugustinMauroy
Copy link
Contributor

I strongly disagree here, even if it's not death. Merging conflicting updates in a reliable way is complex, time consuming to code, prone to errors and results in higher code complexity. And this is completely solved within yjs even with all the editor bindings. Why reinvent the wheel here if you get it for free?

I allow with you. But I still think adding redis on the project is to make it more cumbersome

@JannikStreek
Copy link
Collaborator Author

JannikStreek commented Apr 3, 2024

@AugustinMauroy I pushed a commit where I am trying to connect to an API endpoint in next.js api/room/room_id which is located here: api/room/[id]/route.ts. However, I still get the message:

[next-ws] could not find module for page /api/room/quill-demo-room-websocket-server

Please feel free to play with it. Not sure why it can't find the module, either I am doing something wrong or next-ws has problems supporting dynamic routes (https://nextjs.org/docs/pages/building-your-application/routing/dynamic-routes)? Unfortunately I don't have more time today.

@JannikStreek
Copy link
Collaborator Author

ok last update: the GET request is reaching the api endpoint, but not the web socket request...

@AugustinMauroy
Copy link
Contributor

AugustinMauroy commented Apr 3, 2024

ok last update: the GET request is reaching the api endpoint, but not the web socket request...

I'll take a look it's shouldn't do that.
EDIT:

  1. try to use next-socket as client such as app/socket-demo.
  2. There are a strange behavior, when you are on dev env sometime it's not compile socket route but on build it's work fine.

@JannikStreek
Copy link
Collaborator Author

ok last update: the GET request is reaching the api endpoint, but not the web socket request...

I'll take a look it's shouldn't do that. EDIT:

  1. try to use next-socket as client such as app/socket-demo.
  2. There are a strange behavior, when you are on dev env sometime it's not compile socket route but on build it's work fine.

well, this would make using the existing bindings more complicated (as you can't simply use the yjs web socket adapter) and I honestly do not really understand why the path resolutions isn't working as the correct route is called. But would be interesting to see if that actually works. But I wouldn't consider this for real use.

@SamTV12345
Copy link
Member

ok last update: the GET request is reaching the api endpoint, but not the web socket request...

I'll take a look it's shouldn't do that. EDIT:

  1. try to use next-socket as client such as app/socket-demo.
  2. There are a strange behavior, when you are on dev env sometime it's not compile socket route but on build it's work fine.

well, this would make using the existing bindings more complicated (as you can't simply use the yjs web socket adapter) and I honestly do not really understand why the path resolutions isn't working as the correct route is called. But would be interesting to see if that actually works. But I wouldn't consider this for real use.

The question is what would be the benefit of using next-socket over the yjs implementation.

@JannikStreek
Copy link
Collaborator Author

JannikStreek commented Apr 4, 2024

ok last update: the GET request is reaching the api endpoint, but not the web socket request...

I'll take a look it's shouldn't do that. EDIT:

  1. try to use next-socket as client such as app/socket-demo.
  2. There are a strange behavior, when you are on dev env sometime it's not compile socket route but on build it's work fine.

well, this would make using the existing bindings more complicated (as you can't simply use the yjs web socket adapter) and I honestly do not really understand why the path resolutions isn't working as the correct route is called. But would be interesting to see if that actually works. But I wouldn't consider this for real use.

The question is what would be the benefit of using next-socket over the yjs implementation.

i don't see any but maybe it has to be used so that the routes work properly, we have to test that. Currently I see the following options:

  • We make the routes work, so that we can integrate the web socket backend into the next.js server. Currently stuck because the route is not picked up from web socket calls to next-ws, but normal http requests are. I agree that on first glance this seems to be the most straight forward solution, but we need to make next-ws work. => Help wanted
  • We bring our own small dedicated web socket server (there is already a template in the y-websockets bin folder) that is only serving as a yjs backend. So 2 backend services are running in the end. Also means two services need to share a database connection or we need 2 databases.
  • Integration via y-redis. This already works properly but requires a redis which might be too complicated for smaller etherpad setups. For bigger installations, this might be actually better than the other solutions. Needs some refactoring to clean the y-redis server from authentication JWT token stuff as we don't need that in etherpad.

@AugustinMauroy
Copy link
Contributor

We make the routes work, so that we can integrate the web socket backend into the next.js server. Currently stuck because the route is not picked up from web socket calls to next-ws, but normal http requests are. I agree that on first glance this seems to be the most straight forward solution, but we need to make next-ws work. => Help wanted

I'm more in favor of that. It's more custom things but it's reduce number of backend. We can ask help from maintainer of next-ws ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request infrastructure Pull requests concerning infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: Use of yjs
3 participants