Engine server hook by bruno- · Pull Request #347 · rails/tailwindcss-rails (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation29 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

bruno- marked this pull request as draft

April 20, 2024 20:22

which would break the debugger.

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

Choose a reason for hiding this comment

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

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

needs to only run in development mode, as @bruno- and @brunoprietog discussed in their thread
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.

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?

@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.

@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.

@flavorjones thank you for green-lighting this feature.

I can make changes based on your suggestions.

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.

@@ -2,6 +2,9 @@

module Tailwindcss
class Engine < ::Rails::Engine
config.tailwindcss = ActiveSupport::OrderedOptions.new
config.tailwindcss.server_process = false # Rails.env.development?

Choose a reason for hiding this comment

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

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

@@ -16,6 +17,15 @@
say %( Add <%= stylesheet_link_tag "tailwind", "inter-font", "data-turbo-track": "reload" %> within the tag in your custom layout.)
end

if DEVELOPMENT_ENVIRONMENT_CONFIG_PATH.exist?

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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


module Pidfile
def self.path
Rails.root.join("tmp", "pids", "tailwindcss.txt")

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Why txt?

Choose a reason for hiding this comment

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

This is similar to turning the cache on or off, touch restart.txt, etc.

@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?

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.

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?

I'm not loving the heavy and TW specific ServerProcess implementation here. I think there's definitely something to the idea, but it can't be some TW bespoke thing. It needs to be a generic Rails feature that's then just used here.

Agree. @brunoprietog let me know if you'd like to collaborate on something we could use for Rails generally, happy to make time.

Happy to collaborate here! But the author of this is @bruno-. It's a fun coincidence that we are both named Bruno 😂

dhh mentioned this pull request

Jul 30, 2024

Sorry about that! Autocomplete fail. 🤣

Has anyone already explored the option to have it integrated in the same rails process as a middleware? Basically what sprockets does? Sure, it would be slower, but you can always just run it in a separate thread a-la webpacker