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

Hello World example leverages secret APIs #539

Closed
Sequoia opened this issue Jan 6, 2016 · 9 comments
Closed

Hello World example leverages secret APIs #539

Sequoia opened this issue Jan 6, 2016 · 9 comments
Assignees

Comments

@Sequoia
Copy link

Sequoia commented Jan 6, 2016

http://expressjs.com/en/starter/hello-world.html

var server = app.listen(3000, function () {
  var host = server.address().address;
  var port = server.address().port;

  console.log('Example app listening at http://%s:%s', host, port);
});

docs say nothing about a return value for app.listen, although the code docs do.

In any event that usage is definitely not good for a hello-world example.

@Sequoia
Copy link
Author

Sequoia commented Jan 6, 2016

tried to fork & run site but it's jekyll which I've never been able to run successfully.

@dougwilson
Copy link
Contributor

Hi @Sequoia , that's a good point on our documentation. We do write that

This method is identical to Node’s http.Server.listen().

and so the return value from our method is whatever Node.js returns. It looks like they don't document the return value, either. Besides it getting fixed here, getting their documentation to also mention the return value would also be valuable to the entire Node.js community :)

@crandmck
Copy link
Member

crandmck commented Jan 6, 2016

I agree that we should be more explicit, and I'll pursue clarifying the Node docs as well.

To be fair, the last sentence in the doc does show (in code) the value that it returns:

The app.listen() method is a convenience method for the following (for HTTP only):

app.listen = function() {
  var server = http.createServer(this);
  return server.listen.apply(server, arguments);
};

But I think we should clarify. So, should we just state that app.listen returns an http.Server object?

In any event that usage is definitely not good for a hello-world example.

@dougwilson @hacksparrow Do you agree? Should we change our Hello World example?

@dougwilson
Copy link
Contributor

So, should we just state that app.listen returns an http.Server object?

SGTM

Do you agree? Should we change our Hello World example?

Perhaps. So I think the idea was to get the print statement to show the address and port being listened to. We could eliminate server variable and change the references inside the function to this, but that may also be confusing.

The other option is to just change it to the following:

var express = require('express');
var app = express();

app.get('/', function (req, res) {
  res.send('Hello World!');
});

app.listen(3000, function () {
  console.log('Example app listening on port 3000!');
});

@Sequoia
Copy link
Author

Sequoia commented Jan 7, 2016

👍 @dougwilson's revised version. IMO "hello world" should show the absolute minimum to get an app running, in order to avoid confusion. If server = is there, there's a strong chance readers will assume this is "necessary code" to start an express instance. In the case of server = app.listen(..., in my limited experience this is not "idiomatic" express code-- I don't tend to see this direct reference to the HTTP.Server object used much.

@crandmck That code only shows that app.listen returns whatever server.listen returns, which is likewise not documented! 😸 But I see what you mean.

@crandmck
Copy link
Member

crandmck commented Jan 7, 2016

OK I updated the API doc as noted above: caf6ada

+1 on @dougwilson's suggestion above. I believe @hacksparrow said he'll change Getting Started.

I also opened an issue to update the Node docs accordingly as well.

@hacksparrow
Copy link
Member

Updated the example for all languages, according to Doug's suggestion.

@crandmck
Copy link
Member

crandmck commented Jan 8, 2016

Thanks @hacksparrow !

@Sequoia
Copy link
Author

Sequoia commented Jan 8, 2016

Epilogue: @crandmck submitted issue against nodejs/node, patch should land soon. Thanks for the speedy response yall! 👽

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

4 participants