ether / etherpad-next

Rewrite of Etherpad using NextJS
Apache License 2.0
7 stars 3 forks source link

Added socketio with event logging. #13

Closed SamTV12345 closed 6 months ago

SamTV12345 commented 6 months ago

Description

This pull request adds socket io to NextJS

Validation

You can go to localhost:3000/client and see the connected log in the terminal.

Related Issues

Closes #9

Check List

AugustinMauroy commented 6 months ago

apporch of sub server is wrong IMHO we should use route handler. Why do I think that nextjs route handle is the right solution? It's simply an http server and we're trying to put an http server on an http server.

so have something like that for api:

app/api/chat/route.ts:

import { Server } from "socket.io";

const GET = async (req: Request): Promise<Response> => {
  const io = new Server();

  io.on("connection", (socket) => {
    console.log("a user connected");

    socket.on("send-message", (data) => {
      console.log("message: " + data);
      io.emit("receive-message", data);
    });

    socket.on("disconnect", () => {
      console.log("user disconnected");
    });
  });

  return new Response("Chat server started");
};

export { GET };
SamTV12345 commented 6 months ago

apporch of sub server is wrong IMHO we should use route handler. Why do I think that nextjs route handle is the right solution? It's simply an http server and we're trying to put an http server on an http server.

so have something like that for api:

app/api/chat/route.ts:

import { Server } from "socket.io";

const GET = async (req: Request): Promise<Response> => {
  const io = new Server();

  io.on("connection", (socket) => {
    console.log("a user connected");

    socket.on("send-message", (data) => {
      console.log("message: " + data);
      io.emit("receive-message", data);
    });

    socket.on("disconnect", () => {
      console.log("user disconnected");
    });
  });

  return new Response("Chat server started");
};

export { GET };

This does not work. You would initialize the socket io server everytime a request is made + you would need another port for the socketio things. If you do the pages route you can define a handler but that uses 1 the old syntax + is not very flexible.

So for now this approach is the only solution + we need to initialize a lot of things at server start like settings etc. So we need to control the server itself rather than let NextJS do the whole initialization.

AugustinMauroy commented 6 months ago

This does not work. You would initialize the socket io server everytime a request is made + you would need another port > for the socketio things. If you do the pages route you can define a handler but that uses 1 the old syntax + is not very flexible.

So for now this approach is the only solution + we need to initialize a lot of things at server start like settings etc. So we > need to control the server itself rather than let NextJS do the whole initialization.

I'll continue to read because if you have right we lost a big part of nextjs advantage.

SamTV12345 commented 6 months ago

This does not work. You would initialize the socket io server everytime a request is made + you would need another port > for the socketio things. If you do the pages route you can define a handler but that uses 1 the old syntax + is not very flexible. So for now this approach is the only solution + we need to initialize a lot of things at server start like settings etc. So we > need to control the server itself rather than let NextJS do the whole initialization.

I'll continue to read because if you have right we lost a big part of nextjs advantage.

Why? We can still use NextJS the way we want to for API stuff + views. The only thing that is scraped is that we initialize the server rather than Next initializes it.

I read that Next is made for serverless environment. We don't really need to support that stuff. It's just a standalone webserver.

AugustinMauroy commented 6 months ago

Quite right we lose the fact that nextjs does everything

SamTV12345 commented 6 months ago

Quite right we lose the fact that nextjs does everything

I mean that was clear before. At server start we need to load the settings, initialize our rooms, sockets and load some hooks from the plugins. So we just use the NextJS part for UI+ API. The rest is custom tailored to our needs. I don't really see a problem here.

AugustinMauroy commented 6 months ago

I got one solution we can still use page/api/socket.ts

import { Server as NetServer } from "http";
import { NextApiRequest } from "next";
import { Server as ServerIO } from "socket.io";

import { NextApiResponseServerIo } from "@/types";

export const config = {
  api: {
    bodyParser: false,
  },
};

const ioHandler = (req: NextApiRequest, res: NextApiResponseServerIo) => {
  if (!res.socket.server.io) {
    const path = "/api/socket/io";
    const httpServer: NetServer = res.socket.server as any;
    const io = new ServerIO(httpServer, {
      path: path,
      // @ts-ignore
      addTrailingSlash: false,
    });
    res.socket.server.io = io;
  }

  res.end();
}

export default ioHandler;

That sound little bit more sexy Source

SamTV12345 commented 6 months ago

I got one solution we can still use page/api/socket.ts

import { Server as NetServer } from "http";
import { NextApiRequest } from "next";
import { Server as ServerIO } from "socket.io";

import { NextApiResponseServerIo } from "@/types";

export const config = {
  api: {
    bodyParser: false,
  },
};

const ioHandler = (req: NextApiRequest, res: NextApiResponseServerIo) => {
  if (!res.socket.server.io) {
    const path = "/api/socket/io";
    const httpServer: NetServer = res.socket.server as any;
    const io = new ServerIO(httpServer, {
      path: path,
      // @ts-ignore
      addTrailingSlash: false,
    });
    res.socket.server.io = io;
  }

  res.end();
}

export default ioHandler;

That sound little bit more sexy Source

Except that this does not work. No idea why res.socket.server.io is always undefined.

AugustinMauroy commented 6 months ago

and what do you think about this approach https://github.com/FranciscoMendes10866/soketz/blob/main/app/api/pusher-auth/route.ts

SamTV12345 commented 6 months ago

and what do you think about this approach https://github.com/FranciscoMendes10866/soketz/blob/main/app/api/pusher-auth/route.ts

Well. If you take a look pusher is a commercial product. If you would deploy etherpad-next you would have to register there, create an app, pay for the service at one point and then also hope that they don't get hacked. I'd stick to one server that is hosted within your own infrastructure and no data leaves that server to another web server.

SamTV12345 commented 6 months ago

you have true. So just ping new dep for security (use ~ instead of ^)

Fixed. Yeah I probably tried 2 hours different solutions and then gave up. I mean we aren't Vercel so we don't really need the Serverless part of NextJS.