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

Improve rendering outside request #616

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

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Apr 15, 2024

Follow-up to #529
Closes #613

Remove the & operator from the Turbo::Frames::FrameRequest concern,
since it isn't solving the original issues in the way that it intended.

Instead, add documentation to the README.md that highlights
turbo-rails's compatibility with ActionController::Renderer.

@seanpdoyle
Copy link
Contributor Author

@jwoodrow thank you for sharing more clarifying details to #613.

I've opened this PR to add stronger guarding conditionals instead of using the & safe access operator.

Having fixed the issue, I want to acknowledge that I dislike adding these kinds of conditionals to concerns that are mixed into a controller context, since the presence of a request is crucial to that context and feels like a safe assumption to have been making.

In #613 you've shared some sample code:

ApplicationController.new.render_to_string(
  formats:  [:xlsx],
  handlers: [:axlsx],
  filename:,
  template: 'path/to/view',
  locals:
)

If this call were split up into two successive calls, you'd have an opportunity to assign a request instance:

controller = ApplicationController.new
controller.request = ActionDispatch::Request.new({})

controller.render_to_string(
  formats:  [:xlsx],
  handlers: [:axlsx],
  filename:,
  template: 'path/to/view',
  locals:
)

If you made that change to your tests, would they pass?

Could you share more context about your particular use case? I wonder if there's a different solution to the underlying problem.

@eugeneius
Copy link

Rails provides ActionController:Renderer for rendering templates outside of a request:
https://api.rubyonrails.org/v7.1.3.2/classes/ActionController/Renderer.html

Internally it builds a dummy request object:
https://github.com/rails/rails/blob/v7.1.3.2/actionpack/lib/action_controller/renderer.rb#L124

The sample code from #613 should be rewritten as:

ApplicationController.renderer.render_to_string(
  formats:  [:xlsx],
  handlers: [:axlsx],
  filename:,
  template: 'path/to/view',
  locals:
)

I think instead of this change, #532 should be reverted, since it didn't fix anything and was attempting to support usage that isn't Rails public API.

Follow-up to [hotwired#529][]
Closes [hotwired#613][]

Remove the `&` operator from the `Turbo::Frames::FrameRequest` concern,
since it isn't solving the original issues in the way that it intended.

Instead, add documentation to the `README.md` that highlights
`turbo-rails`'s compatibility with [ActionController::Renderer][].

[hotwired#529]: hotwired#529
[hotwired#613]: hotwired#613
[ActionController::Renderer]: https://api.rubyonrails.org/classes/ActionController/Renderer.html
@seanpdoyle
Copy link
Contributor Author

@eugeneius great point. I've updated this PR to be more focused on documentation instead.

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.

turbo_frame_request_id safe operator in not handled correctly
2 participants