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

Replace render_anywhere gem with ActionController::Renderer to support Rails 6 #55

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

danielpuglisi
Copy link

Hi there,

Stumbled across this Gem yesterday and wanted to implement it in a Rails 6 app but had the same issue as #50 - so I created a quick PR.

Notes:

  • I removed the ActionMailer::Base.default_url_options || {} in favour of using config/initializers/application_controller_renderer.rb. Checkout README update.
  • When running the tests everything passes, but there are some errors coming from the ActiveJob logs. It seems as somehow the ActiveJob specs are using the Sidekiq spec methods of MyTexter#delayed_action – not sure how to fix this though.

set_instance_variables_for_rendering

render(:template => template_name, :formats => ['text'], :locale => @locale)
renderer = ::ApplicationController.renderer.new
Copy link

@no-itsbackpack no-itsbackpack Aug 18, 2019

Choose a reason for hiding this comment

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

@danielpuglisi 👋 I just tested this PR on a rails api I am upgrading to rails 6 and renderer.render keeps returning an empty string because views are not loaded in rails apis. The controllers in rails apis inherit from ApplicationController::API instead of ApplicationController::Base. I was wondering if we could switch to using

renderer = ::ActionController::Base.renderer.new
renderer.render(
  template: template_name,
  layout: false,
  formats: [:text],
  locale: @locale,
  assigns: set_instance_variables_for_rendering
)

which works on both Rails apis and vanilla rails apps

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the input @no-itsbackpack.

True, didn't think about that. I used ApplicationController as Helpers are automatically available this way.

Not sure how to achieve this with ActionController::Base though. Going back to using...

module Textris
  class Base
    class RenderingController < ActionController::Base
      helper :all
    end

  # ...

  def render_content
    renderer = RenderingController.renderer.new

  # ...

... does not seem to load helpers defined in the application.

Choose a reason for hiding this comment

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

Yea I totally didnt consider loading controller helpers, back to the drawing board 🤔

@rafaltrojanowski
Copy link

rafaltrojanowski commented Sep 30, 2019

@no-itsbackpack I'm using your fork with standard Rails 6 project (no-API) and it works without problems! Thanks for your work.

@dandlezzz
Copy link

I'm using this fork and it seems to work great. I did have to make one change,
ondiem@38f0e22.

@vdegove
Copy link

vdegove commented Mar 9, 2021

Hey there! I'd like to use Textris in a Rails 6 project. I liked a lot Textris in a Rails 5.2 project, I found it quite smart and easy to use to mirror mailer mechanisms into SMS.

Do you think it's safe to use this branch in a Rails 6 production environment? Or is there any major issue that prevents this branch from being merged into the main branch and for which you'd discourage to use it?

@danielpuglisi
Copy link
Author

danielpuglisi commented Mar 9, 2021

@vdegove I'm using it in production. I'm not sure if this gem is actively maintained though as no one of the core team responded to this PR so far.

@vdegove
Copy link

vdegove commented Mar 9, 2021

Allright, thanks for the feedback. This gem is way too practical, I'm just gonna fork your branch. I've seen that others have gone the same path even recently like here https://github.com/Semeia-io/textris… Anyway, may be just that folks at visuality.pl didn't even see the PR. @Marahin what do you think about this PR?

@Marahin
Copy link
Contributor

Marahin commented Mar 9, 2021

Anyway, may be just that folks at visuality.pl didn't even see the PR. @Marahin what do you think about this PR?

I am no longer affiliated with the Open Source team at Visuality, but I will ping them on private channels to check it out.

I think the code looks good and I 👍 it.

renderer.render(
template: template_name,
layout: false,
formats: [:html],

Choose a reason for hiding this comment

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

Suggested change
formats: [:html],
formats: [:text],

:formats is [:text] ?

@vdegove
Copy link

vdegove commented May 3, 2021

Hi @Marahin : have you talked to anybody at Visuality to restart maintaining this gem? It's a shame it's not maintained anymore. Or if it's not possible for them, would someone maintain an official fork and just change the README pointing towards this fork?

@rylanb
Copy link

rylanb commented Apr 8, 2022

Maybe #62 fixes the failing tests via Sidekiq here?

Would love to get this merged if possible to support newer versions of Rails. I appreciate this gem and the effort that went into writing it!

@thubamamba
Copy link

Having problems with the gem in Rails 7 which this particular PR can resolve for so many users.

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.

9 participants