search

jlong / serve

Serve is a small Rack-based web server and rapid prototyping framework for Web applications (specifically Rails apps). Serve is meant to be a lightweight version of the Views part of the Rails MVC. This makes Serve an ideal framework for prototyping Rails applications or creating simple websites. Serve has full support for Rails-style partials and layouts.
http://get-serve.com
Other
836 stars 90 forks source link

Do not hardcode port in config.ru #69

Closed ntalbott closed 12 years ago

ntalbott commented 12 years ago

Proposed solution for #49, just removes the hard coded port in config.ru.

bmaland commented 12 years ago

Whats the point of duplicating 108ccc1 in #65 ..? My pull request also runs on port 4000 by default, and fixes serve's inability to run on Heroku Cedar.

ntalbott commented 12 years ago

@bmaland, I actually created this before I saw your more comprehensive patch. That said, the question in this thread has to be answered either way, as removing the hard coding of port 4000 in config.ru does change functionality and I need to understand the desired behavior before I can determine whether that's OK.

bmaland commented 12 years ago

I think the intended logic is port 3000 for Rails apps, and port 4000 for everything else except when overridden by the command line flag (or ENV["PORT"] for Heroku).

jlong commented 12 years ago

Yup that's correct. Any other issues @ntalbott?

jlong commented 12 years ago

Mmmm. So the problem with this is that regular Rack apps currently started on 3000 with Serve. The port override in config.ru makes generated Serve apps startup on port 4000 which matches the existing command line functionality when using Serve in "unstructured" mode.

There's not an easy way to detect Serve rack apps, so I'm not sure of a good solution.

ntalbott commented 12 years ago

After looking deeply at Rack, it seems that it simply isn't possible to achieve the ability to specify a port in config.ru and allow that port to be overridden using the command line. This is due to the way Rack/rackup process options, and wouldn't be a simple fix on that side. Thus I'm going to punt on this and say that if one wants to override the port that's in config.ru, they have to delete the comment that forces the app onto 4000.

jlong commented 12 years ago

Agreed.

bmaland commented 12 years ago

Right, if this is the intended behavior then the docs and the output from "serve --help" would have to be updated, since the functionality described there simply doesn't work.

ntalbott commented 12 years ago

Hmmm, you mean in terms of the --port option? Documenting this nuance is going to be tricky - any ideas @jlong?

bmaland commented 12 years ago

Yeah, both serve --help and http://get-serve.com/documentation/usage are currently misleading about this feature.

I'm still not sure why breaking the functionality of the serve executable is preferred over having config.ru have a port set by default - isn't the most common use case to run serve from the command line?