Implement Engines Support #554
Conversation
876b82b to
d50dae4
Compare
|
@flavorjones, this approach could be a stand-alone gem. I would like to hear from you this week about your preference. I like to put this issue to rest, finally, one way or another. Life is too short to wait for years for that to happen. |
There was a problem hiding this comment.
I've made some comments inline on some small changes, primarily related to the tests.
I'm having trouble testing this end-to-end. I've tried creating my own custom engine and I've tried using your turbo_material gem by adding this to my Gemfile:
gem "turbo_material", git: "https://github.com/full-stack-biz/turbo_material", branch: "feature/tailwind-v4"When I run bin/rails tailwindcss:build[debug] I see that the engine is successfully detected and the engine CSS file is created:
$ find app/assets/builds/tailwind/ -type f
app/assets/builds/tailwind/fake_tailwind_engine_engine.css
app/assets/builds/tailwind/turbo_material.css
and the contents of that look OK:
$ cat app/assets/builds/tailwind/turbo_material.css
/* DO NOT MODIFY THIS FILE, it was auto-generated by tailwindcss-rails */
@import "/home/flavorjones/.local/share/mise/installs/ruby/3.4.2/lib/ruby/gems/3.4.0/bundler/gems/turbo_material-c718ba56f668/app/assets/tailwind/turbo_material/application.css";
However, I don't see any changes in my app/assets/builds/tailwind.css file with and without these engine CSS files.
Any idea what I might be doing wrong?
README.md
Outdated
| # Gemfile gem "tailwindcss-rails", "~> 4.0" # which transitively pins | ||
| tailwindcss-ruby to v4 |
There was a problem hiding this comment.
This is wrapped incorrectly, probably because the linting tool you're using is paying attention to the incorrect code block type html. It should be ruby.
There was a problem hiding this comment.
I suspect it's ToC update that did it. Corrected.
test/lib/tailwindcss/engine_test.rb
Outdated
| test "after_initialize calls Tailwindcss::Engines.bundle" do | ||
| called = false | ||
|
|
||
| # Replace bundle method with our spy | ||
| Tailwindcss::Engines.stub(:bundle, ->{ called = true }) do | ||
| # Execute the after_initialize block | ||
| Rails.stub(:root, File) do | ||
| engine_file = File.join(File.dirname(__FILE__), "../../../lib/tailwindcss/engine.rb") | ||
| engine_code = File.read(engine_file) | ||
|
|
||
| # Verify that the after_initialize block calls Engines.bundle | ||
| assert_match(/config\.after_initialize do.*?Tailwindcss::Engines\.bundle.*?end/m, engine_code, | ||
| "Expected after_initialize block to call Tailwindcss::Engines.bundle") | ||
|
|
||
| # Call bundle to verify our spy works | ||
| Tailwindcss::Engines.bundle | ||
| assert called, "Expected Tailwindcss::Engines.bundle to be called" | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
I don't think this test is meaningful. I would remove it and either
- rely on either integration testing (see
test/integration/), or - skip testing it altogether (if it doesn't work things should blow up rather obviously in a real application)
There was a problem hiding this comment.
I looked over test/integration/ and prefer to remove this test for now. If you ask me opinion on how to approach it, I would suggest a dummy application for that. But transitioning to that in this specific PR feels too much. Maybe I'll offer you a hand in transition separately. Removed for now.
test/lib/tailwindcss/engines_test.rb
Outdated
| File.write(tailwind_dir.join("application.css"), "/* Test CSS */") | ||
|
|
||
| # Add the mock engine to Rails::Engine.subclasses | ||
| Rails::Engine.instance_variable_set(:@subclasses, Rails::Engine.subclasses + [mock_engine]) |
There was a problem hiding this comment.
This does nothing, it should be removed.
lib/tailwindcss/engines.rb
Outdated
| def bundle | ||
| FileUtils.mkdir_p(Rails.root.join("app/assets/builds/tailwind")) | ||
| Rails::Engine.subclasses.select do |engine| | ||
| engine.root.join("app/assets/tailwind/#{engine.engine_name}/application.css").exist? |
There was a problem hiding this comment.
Naming this file "application.css" seems odd given it's the CSS for an engine (not an application). Why not "#{engine_name}/engine.css"? I don't feel strongly about it, just curious.
There was a problem hiding this comment.
Good call, it'll reduce the chances that the developer will mix app/engine files. Done and tested on local.
You need to @import '../../assets/builds/tailwind/turbo_material.css';edit: added note about that to documentation. |
|
I am going to try to address as many comments as possible in the next half an hour, worst case scenario, some will spill over tomorrow (it's late night for me here). |
|
@flavorjones comments are addressed and ready for another round of review. |
I see. I wonder if we should import those for the user .. but we can obviously add that later if it seems useful and we get feedback about it. I'm going to add a small change to the README indicating this is an "experimental" feature, so that we have the freedom to change this behavior if we want to once it's getting used. Otherwise, looks good, will merge. |
|
Thanks @flavorjones , that was my call from the start. I am sure there are lots of scenarios and people who can push it forward; this is just a stepping stone. |
This PR implements support for Rails Engine-specific Tailwind source roots.
Approach
At the start of the host application, we generate import entry points for every Engine that implements
app/assets/tailwind/<engine_name>/application.css.The developer of the host application may use these entry point imports to import them into their specific Tailwind configs.
Motivation
Ideally, I would've preferred to have an analog of
@source "../node_modules/..., but as Rails uses a non-collocated bundler, it's a bit far-fetched to expect support for the retrieval location of gem files in our bundle. This PR offers a developer experience as close as possible to that.app/assets/builds/tailwindfor the generated entry-point@import '../builds/tailwind/<engine_name>in yourapp/assets/tailwind/application.css