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

Use view_helpers of the controller instance instead of standalone module #45

Merged

Conversation

codergeek121
Copy link
Contributor

@codergeek121 codergeek121 commented Dec 23, 2024

I could reproduce this issue by configuring the dummy app as shown in this PR and rendering the root path of the dummy app.

If you're using an asset_host lambda to generate a host based on the request, some view helpers in the middleware fail with an ArgumentError. Before this PR, the middleware used ActionView::Base.helpers, which doesn't know about the current request. This will result in an ArgumentError with an backtrace similar to:

[config/application.rb:29:in `block in <class:Application>'](http://localhost:3000/#)
[actionview (8.0.0) lib/action_view/helpers/asset_url_helper.rb:287:in `compute_asset_host'](http://localhost:3000/#)
[actionview (8.0.0) lib/action_view/helpers/asset_url_helper.rb:213:in `asset_path'](http://localhost:3000/#)
[/hotwire-spark/lib/hotwire/spark/middleware.rb:37:in `script_tag'](http://localhost:3000/#)
[/hotwire-spark/lib/hotwire/spark/middleware.rb:33:in `inject_javascript'](http://localhost:3000/#)
[/hotwire-spark/lib/hotwire/spark/middleware.rb:12:in `call'](http://localhost:3000/#)
[rack (3.1.8) lib/rack/tempfile_reaper.rb:20:in `call'](http://localhost:3000/#)
...

Basically if asset_host is a lambda with 2 or more args, compute_asset_host wants to be called in a request context.

Since we're in a middleware, we have access to the current request, which should allow us to access the asset helpers in a request context. I'm using @request.controller_instance.helpers to access the view helpers, which has the advantage of being in context of the request/controller. This fixes #43.

Maybe this bug doesn't really warrant the dummy app to be reconfigured though 🤔

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

@jorgemanrubia jorgemanrubia merged commit c1ccaa0 into hotwired:main Dec 25, 2024
2 checks passed
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.

config.asset_host with lambda not working
2 participants