Engine server hook

[bruno-]

Hi,

this is a proof of concept for automatically running watch mode alongside any rails server in development.

This is the alternative solution to work done in #300. While puma plugin from that PR works only with puma, this approach works with any server (puma included, of course).

I'm looking for some feedback from the maintainers, also from @npezza93 , @javierav because it affects #327 and #331.

[flavorjones]

I've rebased this branch and added a commit to avoid breaking the debugger (see #346 and #349 for context).

[flavorjones]

I really like this approach, however the final version needs a few things:

1. needs to only run in development mode, as @bruno- and @brunoprietog discussed in their thread
2. needs to only run in new installations

To explain (2) above, this is the scenario we must avoid:

• user installs an older tailwindcss-rails version
• user configures either Procfile.dev and foreman or the Puma plugin to manage the watch process
• user upgrades to a version of tailwindcss-rails with this watcher
• when the user runs their dev server they now have two tailwindcss watchers

One idea is to have the generator create an initializer that sets something like:

    config.tailwindcss_rails_server = true

and then engine.rb can be patched like:

diff --git a/lib/tailwindcss/engine.rb b/lib/tailwindcss/engine.rb
index ead1c9b9..96c87621 100644
--- a/lib/tailwindcss/engine.rb
+++ b/lib/tailwindcss/engine.rb
@@ -15,6 +15,7 @@ class Engine < ::Rails::Engine
     end
 
     server do
+      if Rails.application.config.respond_to?(:tailwindcss_rails_server) && Rails.application.config.tailwindcss_rails_server
         tailwind_pid = fork do
           # To avoid stealing keystrokes from the debug gem's IRB console in the main process (which
           # needs to be able to read from $stdin), we use `IO.open(..., 'r+')`.
@@ -28,3 +29,4 @@ class Engine < ::Rails::Engine
       end
     end
   end
+end

Does that make sense? I'm open to other ideas.

[brunoprietog]

How about a breaking change for a 3.0 version with an update notice? If this is merged, hopefully, we would have 3 ways to run the gem, which means more unnecessary confusion for devs. Couldn't this replace all the paths? Why would we need the other methods having this?

[flavorjones]

@brunoprietog I agree we don't need to support multiple ways of running the watcher in a server if this gets merged, so I intend to remove the other methods, eventually.

However, I'd prefer to separate the introduction of the server hook from the removal of the other methods of running the watcher. Having a deprecation period, even a short one, will allow people to choose when to upgrade. The cost of doing so is not very high, IMHO, and it will save us from having to deal with the support burden.

[flavorjones]

@bruno- Let me know if you want to try to finish this up per my comments above? If not, I'll find time to work on it this week -- I just don't want to duplicate efforts! Thanks.

[bruno-]

@flavorjones thank you for green-lighting this feature.

I can make changes based on your suggestions.

[bruno-]

Quick update - I addressed everything we talked in this thread, but I encountered a problem:

• Puma can be restarted via touch tmp/restart.txt or by sending a USR1 signal to the process.
• It then calls Kernel.exec, reinitializes everything and consequently calls the Engine.server block again - which leads to multiple tailwindcss processes.
• Since Kernel.exec is used, there's no way to maintain a flag like tailwindcss_process_started = true anywhere in the server process. Tailwindcss process doesn't detect changes in the server process.

Some of the approaches I'm thinking about:

• Use tmp/tailwindcss_pid.txt
• Leave this as-is, document this as a limitation/known bug.
• Killing existing tailwindcss subprocess(es) when running server block.

[bruno-]

If (or when) you decide to make "server process" the default for all:

• Just set the tailwindcss.server_process option value to Rails.env.development? (propshaft does it like that)
• Optionally remove the if DEVELOPMENT_ENVIRONMENT_CONFIG_PATH.exist? section from lib/install/tailwindcss.rb

[bruno-]

This will enable the "server process" only for new installs.

I opted for this solution over creating an initializer file because:

• It's one file less to manage
• Config options belong to config/environments/*, not initializers

[bruno-]

Pidfile is a solution to a problem described here: #347 (comment)

[bruno-]

@flavorjones this PR is ready for another look.

[bruno-]

New rails app already have tmp/pids/ dir https://github.com/rails/rails/blob/92591ebd54e044ff3f3b9d802fe65f69cb7e34f2/railties/lib/rails/generators/rails/app/app_generator.rb#L257-L260

[flavorjones]

@bruno- Thanks for continuing your work on this, in particular the care you took around handling Puma restarts. I tried the code out on my local machine and it all seems to work for me.

It's a sizable chunk of new code, though, which is making me hesitate to ship it in this gem, especially since other gems like cssbundling-rails and dartsass-rails might also be able to use this approach to avoid the Procfile.dev/foreman hoop-jumping.

I'm also curious about test coverage; by any chance was this code adapted from an existing project that tests the edge cases (and if so, should we make sure to pay attention to any licensing requirements)?

I would consider putting the process-management code into a new gem, and taking a dependency on that gem, if we can both add some test coverage and use it in other gems. Or: is there an existing gem that does this already? (I searched briefly but wasn't satisfied with the results.)

@rafaelfranca we had a chat a few months back about providing some shared code for the asset builders. Any thoughts here?

[bruno-]

Hi @flavorjones

It's a sizable chunk of new code, though, which is making me hesitate to ship it in this gem

I get it. Let's take a step back and consider our options.

I'm also curious about test coverage; by any chance was this code adapted from an existing project

No, this code was not adapted from any existing project. I started looking at the code in lib/puma/plugin/tailwindcss.rb and took it from there. My only other references were docs.ruby-lang.org and stackoverflow 😅

...from an existing project that tests the edge cases

I, myself spent quite a bit of time manually testing this, discovering bugs etc.

should we make sure to pay attention to any licensing requirements?

I described my working process above. I confirm I'm the author of the code, we can license it however we want.

I would consider putting the process-management code into a new gem, and taking a dependency on that gem, if we can both add some test coverage and use it in other gems.

• I think extracting this into a new gem is a good idea 👍
• Yes, I can add tests.
• Yes, the gem could be used for other rails gems that need "watcher" processes.

Is there an existing gem that does this already?

I'm also not aware of any other gem.

[bruno-]

I'll extract this code to a gem and add tests.

The only question I have is: do you think "server process" is a good name for the gem, and this feature in general?