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

Add support for rails new --server falcon .... #50917

Closed
wants to merge 1 commit into from

Conversation

ioquatix
Copy link
Contributor

@ioquatix ioquatix commented Jan 30, 2024

Motivation / Background

This Pull Request has been created because using falcon (or indeed, any other server, e.g. pitchfork) should be easy.

Detail

This Pull Request changes rails new and adds support for --server puma or --server falcon.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Jan 30, 2024
@@ -281,7 +287,11 @@ def database_gemfile_entry # :doc:
end

def web_server_gemfile_entry # :doc:
GemfileEntry.new "puma", ">= 5.0", "Use the Puma web server [https://github.com/puma/puma]"
if options[:server] == "falcon"
GemfileEntry.new "falcon", ">= 0.28.0", "Use the Falcon web server [https://github.com/socketry/falcon]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, any reason why not to set the >= to the latest version?

@rafaelfranca
Copy link
Member

Thank you for the pull request, but I don't think we want to hardcode more things in the Rails generator. Changing from puma to falcon is just a matter of changing the gemfile and adding a line in application.rb. It isn't worth an option.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jan 30, 2024

@rafaelfranca

I don't think we want to hardcode more things in the Rails generator

Why is that a problem? Is there a better approach we can take?

Changing from puma to falcon is just a matter of changing the gemfile and adding a line in application.rb. It isn't worth an option.

Making it easier for users is a good idea, no? It's definitely easier to explain rails new --server falcon vs "change several files".

I'd like to make it easier for users. As you'll see in the linked issue, this isn't hypothetical, but a real issue users are having.

@eugeneius
Copy link
Member

Also from the linked PR:

I'm wondering if this should be turned on automatically by falcon using a Railtie?

This seems like a better option, since it will make Falcon work correctly even if a user adds it to their Gemfile manually.

@rafaelfranca
Copy link
Member

One problem is that we don't want to be changing Rails every time a new webserver, or database wants to be added here. A better approach would be to add a install command on falcon so you can add the necessary file you need to make falcon to work. Or you can take the approach @eugeneius suggested.

The other problem is that this is basically endorsing falcon, which we don't want to do. Nobody in the core team uses Falcon, so I don't think this is something we can/should endorce.

Just want to point out that we take the same positions with a lot of things. We don't have a rails new foo --test-framework=rspec, and we are going to remove all database adapters that aren't mysql, sqlite and postgres.

@ioquatix
Copy link
Contributor Author

ioquatix commented Jan 30, 2024

This seems like a better option, since it will make Falcon work correctly even if a user adds it to their Gemfile manually.

Yeah, I agree with this and it looks like a great idea, but it's still a separate issue from this PR. I'll need to investigate what's possible.

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

Successfully merging this pull request may close these issues.

None yet

4 participants