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

[discuss continued] how to eliminate manual socket.io initialization #205

Closed
panlina opened this issue Nov 14, 2018 · 8 comments
Closed

Comments

@panlina
Copy link

panlina commented Nov 14, 2018

Hi,

Previously in #200 we already discussed the prefix thing, and that led us to the proposal of a separate .listen method.

Today I'm learning WebSocket and that gives me some idea.

In a app that uses socket.io, there're 2 servers, serving different requests, 1 for http requests, and 1 for socket.io requests. The http.createServer creates the http one, so the other is left to be created some where. Exposing .listen is a direct way, but that is letting the user control the detail. The better way is to do this automatically. We can let the middleware initiate the socket.io stuff, just like http initiates the WebSocket communication.

The difficulty is that socket.io requires a http Server object to initialize, while we do not have it the moment the middleware is constructed. However, the Server object is accessible to the middleware when a request actually happens. I saw it in this SO answer. With this the trick is obvious. We can lazy-initialize socket.io server in the middleware.

I've forked your console-io and did an experiment. After the redesign, no manual .listen call is needed, and the server option is not needed also. You can take a look.

The fork is https://github.com/panlina/console-io.

I've only tried with express 4.

The trick is based on a property that is not documented, and it is warned that it may be dropped in future release, but at least it will work in express 3 and 4, and the advantage it brings is big. Still need to weigh if it is doable.

@coderaiser
Copy link
Owner

That is interesting I'll take a look.

@panlina
Copy link
Author

panlina commented Nov 14, 2018

And 1 more point against this is: if you have plan to support cloudcmd run on connect or Koa in the future, it may be valueless. ☹️

@coderaiser
Copy link
Owner

coderaiser commented Dec 5, 2018

So what you propose to do is add one time condition according to stackoverflow:

 var listening = false;
    router.use((req, res, next) => {
        if (!listening) {
            options.server = req.socket.server;
            module.exports.listen(options);
            listening = true;
        }
        next();
    })

This looks like a hack 🙂 but we already do a similar thing in server/route.js, and not really obvious, but API can be simplified a lot, instead of:

   app .use('/', webconsole({
        server,
        online,
    })).use(express.static(DIR));
    
    webconsole.listen({
        server
    });

Consumers can use:

  app .use('/', webconsole({
        online,
    })).use(express.static(DIR));

But socket server will be initiated on a first request and this can led to slowdown connection.
Have you tried it? Is it much slower or tolerably?

@coderaiser
Copy link
Owner

One more thing that should be considered, is socket is reused in every connected middleware, so we need to provide socket object, and now listen method used for this purpose.

@panlina
Copy link
Author

panlina commented Dec 13, 2018

Being busy these days. I'll give an initial response first. Socket server being initiated on a first request is not a problem I think. I've only tried during development and it didn't show impact. It only affects the first connection anyway. I think it's ok for a admin console app.

Why is the line you showed a similar thing? 😕 I only see a line configuring prefix.

@coderaiser
Copy link
Owner

Why is the line you showed a similar thing? 😕 I only see a line configuring prefix.

You right, similarity in this line only in setting prefix on a request, but web sockets connection initiated in different place.

config('prefix', prefixer(request.baseUrl));

@coderaiser
Copy link
Owner

Closed due to the need of reuse socket object in the middleware.

Also support of express v5 is very important, so we can wait wile it released, or if it's important to you we can proceed in console-io repo if you test how it works with express v5 alpha.

@panlina
Copy link
Author

panlina commented Jan 13, 2019

Haven't been here for a while. 😄 . I'll get back to this when I'm not so busy, maybe after 1 month.

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

No branches or pull requests

2 participants