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

Add builds folder for monitoring #13

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

excid3
Copy link
Contributor

@excid3 excid3 commented Dec 18, 2024

This adds app/assets/builds for monitoring.

Closes #11

@excid3
Copy link
Contributor Author

excid3 commented Dec 18, 2024

The only downside to adding this path instead of replacing is we get duplicate reload requests if say, tooltips.css changes and gets rebuilt into builds.

{"identifier":"{\"channel\":\"Hotwire::Spark::Channel\"}","message":{"action":"reload_css","path":"/Users/chris/code/jumpstart-pro/app/assets/stylesheets/components/tooltips.css"}}
{"identifier":"{\"channel\":\"Hotwire::Spark::Channel\"}","message":{"action":"reload_css","path":"/Users/chris/code/jumpstart-pro/app/assets/builds/tailwind.css"}}

With these gems, we probably want just the builds directory. We could check for tailwindcss-rails, cssbundling-rails, and jsbundling-rails and change the defaults accordingly, but that seems too fragile.

Any ideas on how we could improve that? @jorgemanrubia

lib/hotwire/spark/engine.rb Outdated Show resolved Hide resolved
@mikker
Copy link

mikker commented Dec 18, 2024

Just added this to my local app and it works decently right out of the box w/ tailwindcss-rails.

We "solved" this in hotwire-livereload by adding the option to add a tiny debounce to the change listener (kirillplatonov/hotwire-livereload#59). Not perfect, but works. Tw 4 builds are really fast, so it could probably go down as far as 50 maybe 25 ms.

Because of how tw filters rules by looking at which ones your templates use, you need to wait for it to build before updating for both view and stylesheet changes.

@mikker
Copy link

mikker commented Dec 18, 2024

Fwiw, this works just fine:

  config.hotwire.spark.css_paths = ["app/assets/builds", "app/views", "app/components"]

It fires 3 update events when you update templates but it works just fine.

@jorgemanrubia
Copy link
Member

@excid3 thanks for working on this.

I'd leave the stimulus paths untouched here. As you say, supporting jsbundling will require a more dedicated effort.

For CSS bundling, instead of adding paths, would it be enough to just monitor the builds folder only?

config.hotwire.spark.css_paths = %w[ app/assets/builds ] # not appending, replacing!

That should remove the duplicated signals. Also, with this approach, it doesn't make sense to reload individual stylesheets, you only care about reloading the built CSS, right? I haven't looked into this path properly yet, so maybe I'm missing something.

Here I'm happy with an approach where the engine detects that "css bundling" is used and configures proper defaults here.

@jorgemanrubia
Copy link
Member

We "solved" this in hotwire-livereload by adding the option to add a tiny debounce to the change listener (kirillplatonov/hotwire-livereload#59). Not perfect, but works. Tw 4 builds are really fast, so it could probably go down as far as 50 maybe 25 ms.

Thanks for the insight @mikker. We'll probably need to add some kind of debouncing too at some point. The thing is that it needs to be more fine-grained:

  • For HTML changes, it totally makes sense to debounce.
  • For #nobuild CSS and JSS changes, it doesn't make sense to aggregate changes.
  • For cssbundling and jsbundling, we need see what we do 😅. For starters, for CSS, I wonder if just monitoring the build folder will be enough.

@mikker
Copy link

mikker commented Dec 18, 2024

Considering only Tailwind (and cssbundling similar), there are 2 causes for updating:

  1. User updates their app/assets/stylesheets/application.tailwind.css
    1. CSS update triggered
    2. Tailwind rebuilds css
    3. CSS update triggered
  2. User updates a template, app/views/**/*.html.erb:
    1. HTML update triggered
    2. Tailwind rebuilds css
    3. CSS update triggered

Both could be made into 1 update by debouncing but I can't come up with a good argument for why 2 updates per change is bad enough to make things more complex

@excid3
Copy link
Contributor Author

excid3 commented Dec 18, 2024

I pushed up a new version that just checks for the existence of that folder. cssbundling and tailwindcss-rails both create that directory and possibly other gems, so this would be the most flexible. That said, the builds directory could be used for either JS or CSS.

Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks @excid3. That looks like a good heuristic. Let's see...

@jorgemanrubia jorgemanrubia merged commit fa9f747 into hotwired:main Dec 18, 2024
@excid3 excid3 deleted the add-builds-folder branch December 18, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support app/assets/builds out of the box?
3 participants