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

Refactor start command to use Rackup instead of Puma #875

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

ayushn21
Copy link
Member

@ayushn21 ayushn21 commented Apr 7, 2024

This is a 🙋 feature or enhancement.

Summary

This PR refactors the start command to use Rackup instead of Puma. This way we can run Bridgetown with any Rack compliant server and aren't restricted to Puma.

Rackup will automatically detect from the Gemfile and start it. Our out-of-the-box Puma setup still works as is.

To do:

  • Remove/amend usages of options[:using_puma].
  • Remove hardcoding of host/port and use configuration.
  • Move "Now serving at:" logging to start file and use info from Rackup.
  • Test with another server like Falcon.
  • Test without a server in the Gemfile (Should start with WEBrick I think).
    • Starts with WEBrick, but live reload causes errors. I think we can ignore this since we setup Puma by default.
  • Generic testing.

More details

I had to refactor the way we start the Bridgetown build and aux processes to make it work with this setup. We're now forking a process which runs the build in the background. That in turn starts the aux processes like the frontend watcher. As such I now track PIDs in files so we can kill the processes we start despite stuff happening in different processes.

Context

Fixed #860

@jaredcwhite
Copy link
Member

@ayushn21 I spent a little time with this and it feels pretty solid! Excited to see the progress.

@ayushn21
Copy link
Member Author

I've disabled live reload when using servers other than Puma. It's causing a few issues. Probably a can we can kick further down the road I think.

@ayushn21
Copy link
Member Author

Hey @jaredcwhite could you stress test this PR a bit please? I'm too close to it to test it properly 🙈

@jaredcwhite
Copy link
Member

I think it's looking really great @ayushn21. 🙌 I have a couple of small fixes on hand regarding when the frontend process isn't running (it crashes on Ctrl+C due to missing PID) and just some process messaging on startup. Mind if I push up and we can do a final review, or do you want to step through it ahead of time?

@jaredcwhite
Copy link
Member

jaredcwhite commented Apr 16, 2024

I took the liberty of pushing up one fix, as there was a bug where the watcher in SSR mode wasn't returning from a method before setting up an INT trap, causing automated tests to hang and run indefinitely! 😬

@jaredcwhite jaredcwhite added this to the 2.0 milestone Apr 16, 2024
@ayushn21
Copy link
Member Author

Mind if I push up and we can do a final review, or do you want to step through it ahead of time?

Go ahead and push it :).

I took the liberty of pushing up one fix, as there was a bug where the watcher in SSR mode

Awesome thanks! I was stumped by that when I was working on this but was too tired to figure it out at the time so I kicked the can down the road. Thanks for fixing it!

@jaredcwhite
Copy link
Member

@ayushn21 All right, I pushed up a couple of things…binding to 0.0.0.0 instead of localhost means other devices on the same network can access the site (I use this all the time, like for testing on my phone), and only stopping the frontend bundling if it's actually running (and not in production). Also still want to set the process name so when I run ps I can see a nice readout for the Bridgetown process, next to Puma.

@ayushn21 ayushn21 merged commit 1cdd32b into bridgetownrb:main Apr 23, 2024
3 checks passed
@ayushn21 ayushn21 deleted the rack-spike branch April 23, 2024 16:39
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.

Remove hard dependency on Puma and add generic Rack compliance
2 participants