Skip to content

Add puma plugin#300

Merged
flavorjones merged 2 commits into
rails:mainfrom
npezza93:main
Jan 4, 2024
Merged

Add puma plugin#300
flavorjones merged 2 commits into
rails:mainfrom
npezza93:main

Conversation

@npezza93

@npezza93 npezza93 commented Dec 21, 2023

Copy link
Copy Markdown
Contributor

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

@brunoprietog brunoprietog left a comment

Copy link
Copy Markdown

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.

Comment thread lib/puma/plugin/tailwindcss.rb Outdated
Comment thread lib/puma/plugin/tailwindcss.rb Outdated
Comment thread lib/puma/plugin/tailwindcss.rb Outdated
@npezza93

npezza93 commented Jan 3, 2024

Copy link
Copy Markdown
Contributor Author

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

@npezza93 npezza93 requested a review from brunoprietog January 3, 2024 15:10

@brunoprietog brunoprietog left a comment

Copy link
Copy Markdown

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.

Comment thread lib/install/tailwindcss.rb Outdated
@npezza93 npezza93 requested a review from brunoprietog January 3, 2024 19:45
@brunoprietog

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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

npezza93 commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Member

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

@npezza93

npezza93 commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

@flavorjones Should be all set!

@flavorjones

Copy link
Copy Markdown
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.

@airblade

airblade commented Jan 5, 2024

Copy link
Copy Markdown

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
Copy Markdown

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

hdittmar commented Jan 15, 2024

Copy link
Copy Markdown

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.

5 participants