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 puma plugin #300

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Add puma plugin #300

merged 2 commits into from
Jan 4, 2024

Conversation

npezza93
Copy link
Contributor

@npezza93 npezza93 commented Dec 21, 2023

This adds a puma plugin that runs watch mode only in development.

Copy link

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

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

This is great! Goodbye bin/dev. It would be nice to change the install generator to default to include the plugin in the Puma configuration in the development environment.

lib/puma/plugin/tailwindcss.rb Outdated Show resolved Hide resolved
lib/puma/plugin/tailwindcss.rb Outdated Show resolved Hide resolved
lib/puma/plugin/tailwindcss.rb Outdated Show resolved Hide resolved
@npezza93
Copy link
Contributor Author

npezza93 commented Jan 3, 2024

@brunoprietog Should have everything addressed! I also removed the bin/dev and procfile stuff. Let me know if you want to keep that around

Copy link

@brunoprietog brunoprietog left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer, I just leave my comments that I think could be useful to help this merge more quickly.

I would really love to leave bin/dev, and actuallty this same change could be very good for dartsas-rails, jsbundling-rails and cssbundling-rails if the maintainers agree.

I think it would be good to add a description to the PR. Leave bin/dev is a very convincing argument at least for me.

lib/install/tailwindcss.rb Outdated Show resolved Hide resolved
@brunoprietog
Copy link

Nice! I have no further comments from me at least. Hopefully it will merge soon and we will have the same for the other gems that force us to use bin/dev.

@flavorjones
Copy link
Member

I think I understand why adding Puma support is valuable; but what's the rationale for deleting lib/install/Procfile.dev and lib/install/dev? That will break the developer experience for people using foreman today.

@brunoprietog
Copy link

It won't break it, it would only be for new applications. bin/dev is horrible for debugging, and I personally don't like using foreman because it adds extra information that I don't need in the terminal. You should only need to run bin/rails server and that's it.

@flavorjones
Copy link
Member

There is a developer workflow that is well-documented and expected to work. Unless the files prevent users from using a puma plugin, they should not be removed. And if they do, I want to understand why.

@brunoprietog
Copy link

How could you debug using bin/dev? Whenever I need to debug, I have to use rails server separately. That for me at least is quite annoying.

In my opinion, everything should just work in a single terminal. Why would you want to have two ways if one works better than the other? I don't understand why it would be better to use bin/dev.

I mean, why would you want to use foreman? That means adding one more dependency. It also means that in some cases, newvie developers have to use bin/dev and in others rails server. Explaining this to a newbie coming from node.js for example is not so easy, being that they can run everything in a single process there.

@npezza93
Copy link
Contributor Author

npezza93 commented Jan 4, 2024

@flavorjones How would you feel about reverting all but the puma plugin and have the discussion around bin/dev in a separate PR?

@flavorjones
Copy link
Member

@npezza93 Yup, that would be great, thank you!

@npezza93
Copy link
Contributor Author

npezza93 commented Jan 4, 2024

@flavorjones Should be all set!

@flavorjones
Copy link
Member

@npezza93 Thank you! I've squashed your commits to leave a clean history, and I've reworked the README a bit to feature the Puma plugin more prominently.

This is great work, thank you! Worked great for me when I tried it out on my local dev environment.

@flavorjones flavorjones merged commit 3041a5b into rails:main Jan 4, 2024
22 checks passed
@airblade
Copy link

airblade commented Jan 5, 2024

I mean, why would you want to use foreman?

@brunoprietog So you can spin up other processes too, such as Redis or Sidekiq.

@brunoprietog
Copy link

Redis can be started as a service just like PostgreSQL or MySQL. And Sidekiq you can also manage it with puma, so a single command is enough and you can easily debug.

@hdittmar
Copy link

hdittmar commented Jan 15, 2024

I tried adding the plugin :tailwindcss... configuration line mentioned in the Readme into my puma.rb file and since then, puma crashes on launch, for more info see here: #319

(Problem solved, happened due to a conflict with the sassc-rails gem)

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

Successfully merging this pull request may close these issues.

None yet

5 participants