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

callback for listen ? #5

Open
darkyen opened this issue Jan 10, 2016 · 7 comments
Open

callback for listen ? #5

darkyen opened this issue Jan 10, 2016 · 7 comments

Comments

@darkyen
Copy link

darkyen commented Jan 10, 2016

Listen might throw error, and can be fairly non-sync. I don't think there is error mechanism in the library so far, callback for listen maybe ?

@iefserge
Copy link
Owner

Callback might be hard to support for node, because it has listening event, but no "listening error" event. Would this work? Basically the same events as in node:

server.onerror = err => console.log('some server error');
server.onlistening = () => console.log('listening');

@darkyen
Copy link
Author

darkyen commented Jan 10, 2016

I'd prefer extending an EventEmitter for Server instead of onerror, onlistening, anyhow it needs to be aware of the listen callback from the backend.

I was hoping something like this

function listen(handle, port, callback){
     handle.listen(port);
     handle.once('listen', callback);
}

or even better with promises

// proimisified by bluebird.
function listen(handle, port, callback){
     return handle.listenAsync(port);
}

As mentioned here https://nodejs.org/api/net.html#net_server_listen_path_backlog_callback
node does support listen event in the form

server.listen(path[, backlog][, callback])

Then with the power of ES 7 async/await we can simply just make them completely async calls. Even without ES7 the promise way in general will make your server code much cleaner :-) and we can pipe promises.

@iefserge
Copy link
Owner

This lib doesn't use EventEmitter, so events are handled by properties like onclose, onerror that support one possible listener. Arguably makes it more portable, one less dependency I guess.
Node's listen() has a callback, but it's the same as attaching listening event, it doesn't really handle possible errors. Otherwise, I think promises (and es7 async of course :) ) would be great for this.

something like this for Node:

function listen(handle, port, cb) {
  handle.listen(port, function() {
    server.onlistening();
    cb();
  });
}

@darkyen
Copy link
Author

darkyen commented Jan 11, 2016

I am forking the lib, to add event emitter which are supported natively in node and mostly in browsers and visionmedia/debug support expect PR 👍 :-)

@iefserge
Copy link
Owner

I'd keep it without event emitter, it's a little faster and it's one less dependency.

@darkyen
Copy link
Author

darkyen commented Jan 12, 2016

After looking at runtimejs.org I understand why you wish to keep this extremely low dependency.

@adminy
Copy link

adminy commented Mar 18, 2019

@darkyen I have no idea what you’re talking about, runtimejs.org is filled with dependencies everywhere. You even have files that require each other in the project. If you want to have the event emitter why not use express server? It’s got everything you need. This is a very nice http library because it’s so adaptable. If you could make it parameter passed library I don’t see a problem with that, but don’t make it require it out of the box, it removes it’s portability.

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

No branches or pull requests

3 participants