Feature/no request context

[eacaps]

I recently moved our project from browserify to webpack using your tutorial. Great job on that! However I ran into an issue when attempting to use the react_component helper inside an actionmailer. The react_on_rails_helper.rb rails_context method expects to always come from a context that has a request. Unfortunately this is not the case when creating an e-mail via an ActionMailer. I did some basic modifications of react_on_rails_helper.rb to mitigate this issue and included a full test case in your test suite to handle this. You can see what was happening earlier by checking out this commit and running the tests. I'm obviously open to some tweaks to my implementation of rails_context since right now if there's no request it will just return an empty object.

Thanks again for your great work on react_on_rails, hopefully this will help.

---

This change is

[coveralls]

Coverage increased (+0.05%) to 82.353% when pulling 6fdb73a on eacaps:feature/no-request-context into e6afa98 on shakacode:master.

[justin808]

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

---

app/helpers/react_on_rails_helper.rb, line 348 [r1] (raw file):

    @rails_context ||= begin
      result = {}
      unless request.nil?

how about

if request.present?

---

spec/dummy/app/mailers/dummy_mailer.rb, line 4 [r1] (raw file):

  add_template_helper(ReactOnRailsHelper)
  default from: "nobody@nope.com"
  # layout 'mailer'

what is this comment?

---

spec/dummy/app/views/dummy_mailer/hello_email.html.erb, line 7 [r1] (raw file):

        name: "Mr. Mailing Server Side Rendering"
    }
}, prerender: true) %>

needs line return -- see the red symbol

---

spec/dummy/config/initializers/react_on_rails.rb, line 5 [r1] (raw file):

  # all calls to react_component and redux_store for rendering
  def self.custom_context(view_context)
    result = {}

could we do this without a rescue?

is there something in view_context that tells us we're in a mailer. This is the Rails API. Introspect on view_context in the debugger, like in pry.

We also need to update the docs on the railsContext for this case, and in general for this fix. We should put in some flag that we're in mailer, like

{
  mailer: true | false;
}

---

Comments from Reviewable

[justin808]

Nice job @eacaps! Some comments!

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.05%) to 82.353% when pulling 840bc62 on eacaps:feature/no-request-context into 9a8b54f on shakacode:master.

[coveralls]

Coverage increased (+0.05%) to 82.353% when pulling 840bc62 on eacaps:feature/no-request-context into 9a8b54f on shakacode:master.

[eacaps]

@justin808 I've made the first few tweaks you recommended. The last suggestion relates to a special custom_context that was already being added to the dummy tester. I tried several ways to get around using a rescue block but nothing really seems to work properly (.try, .respond_to, etc..). Accessing session from view_context uses a delegate from the ActionView::Helpers::ControllerHelper and since the base ActionMailer does not have a session method you end up with: undefined method `session' for #DummyMailer:0x007fe1c8e58f10. Since this is in the testing code and not the actual library I felt okay leaving the rescue block in there, but if anyone has another suggestion please feel free to make it.

As far as improving the railsContext in this particular case those changes would be made in the react_on_rails_helper.rb in the rails_context method. The first thought that comes to mind would be changing the initial result to be

result = { mailer: self.controller.kind_of?(ActionMailer::Base) }

I haven't actually implemented that yet, but let me know if that all makes sense.

[eacaps]

Also I found this ancient rails issue related to ActionMailer delegation: rails/rails#15613. No action since 2014 but it describes the issue.

[justin808]

Let's go with a property called inMailer, as mailer sounds like more than a boolean.

Maybe to avoid the exception, we can have the customization of the railsContext check this inMailer flag?

happy hacking! this is looking good!

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

spec/dummy/config/initializers/react_on_rails.rb, line 5 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

could we do this without a rescue?

is there something in view_context that tells us we're in a mailer. This is the Rails API. Introspect on view_context in the debugger, like in pry.

We also need to update the docs on the railsContext for this case, and in general for this fix. We should put in some flag that we're in mailer, like

{
  mailer: true | false;
}

@eacaps Did you get a chance to hack on this, to figure out from the view_context as to when we're in a mailer? This is something we should document.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.05%) to 82.353% when pulling 52f0cbb on eacaps:feature/no-request-context into 8ca86ed on shakacode:master.

[justin808]

Looking good!

Please squash your commits and address the following issues.

be sure that rake lint passes, as well as the full test suite rake

---

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

app/helpers/react_on_rails_helper.rb, line 347 [r4] (raw file):

  def rails_context(server_side:)
    @rails_context ||= begin
      result = { inMailer: controller.present? && controller.is_a?(ActionMailer::Base) }

Did this pass the linter? I'd probably rather see this on 3 lines.

---

app/helpers/react_on_rails_helper.rb, line 365 [r4] (raw file):

          # Locale settings
          i18nLocale: I18n.locale,

can we move the I18n parts to outside the request.present? block?

---

spec/dummy/app/mailers/dummy_mailer.rb, line 4 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

what is this comment?

OK

---

spec/dummy/app/views/dummy_mailer/hello_email.html.erb, line 7 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

needs line return -- see the red symbol

OK

---

spec/dummy/config/initializers/react_on_rails.rb, line 12 [r4] (raw file):

result = {}

Quoted 4 lines of code…

unless view_context.controller.is_a?(ActionMailer::Base)
  result = {
    somethingUseful: view_context.session[:something_useful]
  }

    end result

how about

if view_context.controller.is_a?(ActionMailer::Base)
  {}
else
    {
        somethingUseful: view_context.session[:something_useful]
      }
    end

---

Comments from Reviewable

[justin808]

@eacaps Any update?

[coveralls]

Coverage increased (+0.04%) to 82.338% when pulling 3c0e6d9 on eacaps:feature/no-request-context into 63436ab on shakacode:master.

[coveralls]

Coverage increased (+0.04%) to 82.338% when pulling 3c0e6d9 on eacaps:feature/no-request-context into 63436ab on shakacode:master.

[eacaps]

I was having some trouble squashing my commits correctly; it seemed to work in another test branch I created but this one wasn't doing it properly. Perhaps because I merged master in a couple times? I was using this link: http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html. If you know what I need to do I can try again, or we could close this pull request and open one with a squashed branch. Let me know.

[coveralls]

Coverage increased (+0.04%) to 82.338% when pulling 7d4e7fe on eacaps:feature/no-request-context into 63436ab on shakacode:master.

[justin808]

---

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[justin808]

Hi @eacaps! Looking good.

I think the easiest way to squash is to do a git merge --squash over to a new branch, then once you see the branch is correct (tests pass, etc.), delete the original branch, then rename the new branch to the original branch name. Then do a force push.

Please see https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md for a checklist of things to do.

I noticed that we're missing the changelog entry.

Thanks!

[eacaps]

@justin808 I successfully squashed things into a new branch, but in the process this pull request closed on me. I don't know if its possible for you to reopen this or if I need to submit a new pull request?

[justin808]

@eacaps This was already merged in #509

[Xeboc]

I'm getting a:

'NameError in PagesController#index' 
undefined local variable or method `controller' for #<PagesController:0x007f022cf7fed8>
Did you mean? controller_path

After upgrading to 6.1.0 from 6.0.5. The framework trace points to this line: 348

+        inMailer: controller.present? && controller.is_a?(ActionMailer::Base),

config/initializer/app_renderer.rb

ActionController.add_renderer(:app) do |data, options|
  self.extend ReactOnRailsHelper
  self.extend ActionView::Helpers::TagHelper

  component = options.fetch(:component, 'App')
  html = react_component(component, props: data)
  render(options.merge(inline: html))
end

app/controllers/pages_controller.rb

class PagesController < ApplicationController
  def index
    render app: hydrate_data
  end
end

Maybe I'm missing something or using the tooling wrong?

[justin808]

@Xeboc what version of Rails. The code works with with 4.x and 5.x Rails.

[Xeboc]

Rails 5.0.0.1

[justin808]

@Xeboc I'll need you to dig into this further. I'm happy to take a PR if you've got an issue.

[Xeboc]

I'll dig, and if looks like it needs a PR, I'll make one. I wanted to share with you and @eacaps to see if you had any input. Sometimes code and errors just need another set of eyes for someone to go "Oh, look! Right there! Change that!", or to make a slight tweak on their end for robustness. Thank you for the project and the work you've put into it.

[justin808]

@Xeboc Big thanks! My team is slammed with helping our paying clients. We are available for modestly priced coaching packs if anybody is interested: http://www.shakacode.com/work/.