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

Implement Engines support #484

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bopm
Copy link

@bopm bopm commented Jan 29, 2025

This PR implements support for Rails Engines Tailwind styles. It does not require configuration, but sets basic conventions and allows users to redefine engine styles in the host application.

@flavorjones
Copy link
Member

Thanks for thinking about this!

I'd like to land v4 support before I think about what's next, so it may be a day or two before I can review and give good feedback.

@bopm
Copy link
Author

bopm commented Jan 29, 2025

Totally, this needs v4 to work. Ping me when you get to this, I am happy to add missing parts including tests.

@bopm
Copy link
Author

bopm commented Feb 10, 2025

@flavorjones any updates for your expectations here?

@flavorjones
Copy link
Member

@bopm I appreciate your patience. Reviewing this PR is near, but not at, the top of my list and this is going to be a very busy week for me.

@bopm
Copy link
Author

bopm commented Feb 10, 2025

Sure, once again, take your time. I only keeping hand on a pulse to be sure it's not blocked by you possibly waiting on something from me.

@flavorjones
Copy link
Member

@bopm Thanks for your patience, and thanks for putting together this PR!

I think this is the right general direction! But because I think our mental models aren't in sync yet, I wanted to talk through some of the things that came to mind while I read the code.

Clarifying the use case

In the discussion thread, there were a few use cases brought up, including (but not limited to):

  1. generating a separate tailwind.css file for each engine (comment, comment)
  2. generating one single tailwind.css that encompasses the app and all its mounted/referenced engines (comment)

It feels like this PR is aimed at the second problem description, which is fine and is where DHH aimed the discussion. I just want to make sure we're being clear that we're solving that problem and not the first one.

(And, to be even more clear, I think the first problem (gems like ActiveAdmin) can probably be solved very simply using tailwindcss-ruby directly. If someone has tried this and it hasn't worked properly, please drop a note in the discussion.)

Finding engines

We should probably only be looking at mounted engines, so Rails::Engine.subclasses is a bit too broad of a search in my opinion.

But stepping back a moment, I had originally suggested a railtie configuration for engines to explicitly register with this gem, rather than this gem having to go out and find relevant engines. What do you think of using initializers for this instead of class and file existence?

Or, 🤯 if we really want to embrace the tailwind model of "zero config", why not just add every mounted engine's root path to the tailwind search path? If the engine doesn't have tailwind CSS styles, no big whoop, tailwind seems fast enough and smart enough to figure it out, and that implementation would be simpler both for this gem but also for the engine maintainers.

Is @import the right tool?

I haven't had to include external engines yet, and so I wanted to ask: Does @import do what's needed here?

I had imagined that we would need to add the the engine root directory to the list of directories searched by tailwind, perhaps using the @source rule.

Is the expectation that the application.css that we import does this source declaration itself? Seeing an end-to-end example would be really helpful to understand both the problem you're trying to solve and how this proposal would solve it, I just haven't played with it enough yet.

I realize creating an end-to-end example (preferably in an integration test suite script like we do for installation and upgrade) is a lot of work, and you were probably waiting for feedback before putting in the additional effort, but I do think we're on the right path here so maybe it would be worth the investment.

Temporary file versus editing the file that's in source control

This PR creates a temporary file with the additional engines' configuration, and rewrites the command that is run.

I think I would prefer to modify files that are expected to be checked into source control so that it's clear what's happening and so it's easily debuggable by the app owner or engine maintainer.

The simplest pattern I can think of is to use a new file for the engine config. Add something like @import "engines.css" to application.css once, and then rewrite the engine config into app/assets/tailwind/engines.css every time the build command is run. WDYT?


Thanks again for this PR, I do appreciate your time and patience.

@bopm
Copy link
Author

bopm commented Feb 19, 2025

While I'll need some time to address the whole set of questions, allow me to answer on

Is @import the right tool?

Here is pointer towards it from a Tailwind team member. And I tend to agree. If we use @source instead of @import, we will limit engine authors' ability to decide what belongs and does not belong in the Tailwind content.

By using @import we allow the engine author to use multiple @source directives in his app/assets/tailwind/ENGINE_NAME/application.css to define any needed config, including dependency on external non-rails assets for example.

@flavorjones
Copy link
Member

flavorjones commented Feb 19, 2025

Thanks for helping me understand the import-vs-source distinction.

we will limit engine authors' ability to decide what belongs and does not belong in the Tailwind content

For engine authors who don't care, though, would @source be appropriate? I'm asking these questions to help inform DX design tradeoffs. Specifically, I'm mentally exploring if this gem could either @import a CSS file or @source the engine root directory based on config params or file existence (or something else).

@bopm
Copy link
Author

bopm commented Feb 19, 2025

we're solving that problem and not the first one.

Yep, we trying to generate single file from all engines and host app involved.

first problem (gems like ActiveAdmin) can probably be solved very simply using tailwindcss-ruby directly.

What I have in mind as one of examples is possible scenarios for engines extending each other. Let's say I have free engine, and commercial extension to that. I like them both processed in a orderly manner and give me styles.

We should probably only be looking at mounted engines, so Rails::Engine.subclasses is a bit too broad of a search in my opinion.

I tend to disagree here. Here is my another example. I am not mounting anything, but provide component library. Component libs are kinda really popular these days. Using fact that engine is dependent on our gem is good way to be specific. No reason for engine authors to include that dependency if they not produce Tailwind code.

What do you think of using initializers for this instead of class and file existence?

Configuration instead of convention. As long as engine authors conform, host app developers don't have to do manual work for them.

why not just add every mounted engine's root path to the tailwind search path?

As I mentioned in previous answer, we passing the torch to engine authors. As long as they use v4 and @source configuration it'll be natural.

Seeing an end-to-end example would be really helpful to understand both the problem you're trying to solve and how this proposal would solve it, I just haven't played with it enough yet.

Here is example:

  • Lib
  • Host applicationapp/assets/tailwind/application.css head section:
@import 'tailwindcss' source(none);

@source "../../javascript/**/*.js";
@source "../../../views/**/*.{erb,haml,html,slim}";

@theme {

I realize creating an end-to-end example (preferably in an integration test suite script like we do for installation and upgrade) is a lot of work, and you were probably waiting for feedback before putting in the additional effort, but I do think we're on the right path here so maybe it would be worth the investment.

I am going to try, as soon as we agree that overall approach is solid and on the road to be merged.

checked into source control

Here is problem. Bundler does not have executable callbacks on installs/updates. So you will have to run manual task on every possible bundle update to update that file. And as long as you not building everything in Docker, you may get into situation where what you committed into source control is different by gem source roots from your build environment. Ruby version change will broke it too. Too many variables in getting these engines roots. Reflection is more trusted way if you ask me. And requires no oversight.

so it's easily debuggable by the app owner or engine maintainer.

Debugging is kinda easier than it may look:

bin/rails 'tailwindcss:watch[verbose]'
["/Users/sergeymoiseev/.rbenv/versions/3.4.1/lib/ruby/gems/3.4.0/gems/tailwindcss-ruby-4.0.0-arm64-darwin/exe/arm64-darwin/tailwindcss", "-i", "/var/folders/5b/2y5y0p_j575b6mpq1wvbyrz40000gn/T/tailwind.css20250219-86561-96647n", "-o", "/Users/sergeymoiseev/APP_NAME/app/assets/builds/tailwind.css", "--minify", "-w"]

and then in separate terminal while command is running

cat /var/folders/5b/2y5y0p_j575b6mpq1wvbyrz40000gn/T/tailwind.css20250219-86561-96647n
@import "/Users/sergeymoiseev/.rbenv/versions/3.4.1/lib/ruby/gems/3.4.0/bundler/gems/turbo_material-9f6aa17c1a44/app/assets/tailwind/turbo_material/application.css";
@import "/Users/sergeymoiseev/APP_NAME/app/assets/tailwind/application.css";

but we clearly can add file contents to verbose output for better DX

The simplest pattern I can think of is to use a new file for the engine config. Add something like @import "engines.css" to application.css once, and then rewrite the engine config into app/assets/tailwind/engines.css every time the build command is run. WDYT?

Clearly we can go this way, to remove temp file processing and give developer this file in exact location to investigate even if you killed watch command or your build ended, but probably we need to agree that we adding this file to .gitignore by default (due to the fact that build will generate it differently on every engine update)

@source the engine root

First thing that comes to mind, is that typical engine has dummy test app, which may have it's own content we do not want to process. Plus what do we gain even if author don't care? They still probably have sources setup in engine, and we only have to include from host.

@bopm
Copy link
Author

bopm commented Feb 19, 2025

and @flavorjones I appreciate your feedback and need for understanding. I really hope to arrive to the level when we going to have educated mutually agreed approach.

@flavorjones
Copy link
Member

I appreciate your thorough response!

What do you think of using initializers for this instead of class and file existence?

Configuration instead of convention. As long as engine authors conform, host app developers don't have to do manual work for them.

Sorry, to be clear I meant the engine developer would write the initializer in the engine's railtie, not the host app developer. But I get your point about conventions.

Here is my another example. I am not mounting anything, but provide component library. Component libs are kinda really popular these days.

OK, seeing a live example of a non-mounted engine is really valuable, thank you for pointing me at it. I'd like the opportunity to play with this a bit to update my mental models, because the lessons you've learned are already encoded here. Let me read the code and come back to this thread better prepared.

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.

2 participants