Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Disable roadie on demand #31

Closed
greggroth opened this Issue Jul 27, 2012 · 28 comments

Comments

Projects
None yet
4 participants

Roadie is slow and if you want to show a preview of a message before sending it (and don't need inline styles since it's in-browser), it can really slow down the request (add a couple hundred of ms per mailer). Is there a way to disable roadie by request so the mailer previews don't inline the styles while the actual mailer (handled by Sidekiq) has inline styles?

Owner

Mange commented Jul 27, 2012

Excellent feature request. I think you can get this working right now by passing nil as the css option when generating the mail. You could introduce a new parameter to your mailer that expose this toggle.

Let me know if this doesn't work and I'll see if I can find another workaround.

I will see about implementing a proper feature when I can.

I'll try to give it a look over the weekend and maybe come up with a pull request for you. 😉

I need this feature for speeding up testing!
How about introducing a config so that in testing environment roadie is turned off?

Owner

Mange commented Sep 17, 2012

That could work. It's not possible for you to integrate this manually by overriding the css option, though? Like this:

class SomeMailer < ActionMailer
  def some_message(user, inline_css = true)
    css_name = (inline_css ? 'email' : nil)
    mail(to: user.email, css: css_name)
  end
end

or perhaps like this:

class SomeMailer < ActionMailer
  cattr_accessor :skip_css_inlining

  def self.skipping_css
    old_value = self.skip_css_inlining
    self.skip_css_inlining = true
    yield
  ensure
    self.skip_css_inlining = old_value
  end

  def some_message(user)
    mail(to: user.email, css: css_name)
  end

private
  def css_name
    return nil if self.class.skip_css_inlining
    'email'
  end
end

# in the tests
around(:each) do |example|
  SomeMailer.skip_css { example.run }
end

If you want it turned off for all(!) tests, you could make the require 'roadie' call manually in an initializer that checks the environment first. I would not recommend this, though, since your tests should catch any markup/css parser errors.


Overall, I'm getting a bit tired about how magic and automatic Roadie is. It's nice for beginners, but for any project that is non-trivial all the magic get in the way. If anyone have any suggestions on how to make this easier for everyone, I'm all ears.

Here's a thought:

class MyMailer < ActionMailer
  # include Roadie::AutomaticInliner
  include Roadie::MailerHelpers

  def some_message
    # mail(...)
    roadie_mail(...)
  end
end

This would leave it up to the user how the mailing should happen. It would be somewhat easy to introduce a custom module inside any project that wants this configurable.

module ConfigurableInliner
  include Roadie::MailerHelpers

  def mail(*args)
    if inline_css?
      super
    else
      roadie_mail(*args)
    end
  end

  def inline_css?
    # whatever fits your app
  end
end

Now I am using this to turn off Roadie (not really turn off, just give nil for css option)

def roadie_css
  # In test environment we do not need CSS
  return nil if Rails.env.test?
  :mailer
end

ActionMailer's default method store the value
instead of calling the value at runtime

So if you want to make Roadie more configurable, using that default method is not possible

Maybe there should be a subclass called RoadieMailer inherit from ActionMailer
Then add class config, override the methods, etc

@Mange I like your idea of including Roadie::MailHelpers and/or Roadie::AutomaticInliner inside an ActionMailer. This seems like a great option for beginners and for complex apps that need Roadie's functionality in some mailers but not others.

I thought the CSS option also supports Proc object like ActionMailer

But it doesn't =3=
Maybe that one is easier to implement? :3

Edit 1: Found the code

Owner

Mange commented Sep 25, 2012

Try the master branch and see if that works better for you. :-)

Need to do it this way:

default css: lambda {UserMailer.roadie_css}

Then

def self.roadie_css
  # In test environment we do not need CSS
  return nil if Rails.env.test?
  :mailer
end

Or it cannot find the method :P

Good for choosing style, but it will be better if we can have a kill switch for testing (default: on)

Owner

Mange commented Sep 27, 2012

Sure, and some way to turn it off completely should be coming. At least you can turn off stylesheets now, which should speed up tests a lot.

RE: the lambda stuff; I don't change any scope for the lambas. Perhaps I should, but I think the default behavior of lambdas should be kept to keep people from becoming surprised by the behavior.

I'm surprised it cannot find the method without the explicit class name, though. That should just work... :-/

This works for me in 1.9.3 irb:

class Foo
  class << self; attr_accessor :proc; end
  self.proc = lambda { foo }

  def self.foo() "foo" end
  def test() self.class.proc.call end
end

Foo.new.test # => "foo"

I guess ActionMailer does something weird with the defaults options... Does this work for you?

def self.roadie_css() end
default css: method(:roadie_css)

I though I posted something but it disappear!?
Whatever
It does not work since with error message (no method error "bind")

Found this code in actions mailer
They use v.bind(self).call
But in your code it is just target.call

Since the method resolve_target is within ActionsMailer
Maybe you can try target.bind(self).call?

Owner

Mange commented Sep 30, 2012

I see. Well, this is one of those cases where it's hard to tell which is the right way to do it. Either you make it work in the context of the instance when defined in the class but destroy the concept of the closure, or you keep the context there so it works like it should.

I'm speaking on Rail's behalf, though. Of course I'll bend to keep my extensions working the same as the other Rails code. I hope to patch this during the coming week.

Maybe I can see how does other "Rails extensions" do it
But in the case ActionMailer I think it's better to make it works like ActionMailer does
Also you can already ignore the Class prefix if you call it inside mailer method

Hmm any news?

Right now the easiest thing to do is to add an option to your class
And make CSS injection method check the config before doing the real work

Owner

Mange commented Oct 26, 2012

Sorry about the lack of progress. You should try master in your app and see if that makes it better for you.

Oh sorry I didn't check the code change first :3
I will try it next week, developing our own jQuery tagging plugin right now _

Owner

Mange commented Oct 26, 2012

Oh sorry I didn't check the code change first :3

I just wrote it after you commented. ;-)

Oh
Anyway enjoy Halloween :)
🎃

OK I have tested it

Setup

  • Add config.roadie.enabled = false if Rails.env.test?
  • Run 400 simple mail rendering
    (insert 100 render call in a method called in 4 examples)

Result

  • config.roadie.enabled = true
  • Finished in 11.72 seconds

Off

  • config.roadie.enabled = false
  • Finished in 10.86 seconds

Comment

  • 1 sec gain lol
  • But I don't have much style in this new project, I will try this in the old project
Owner

Mange commented Oct 31, 2012

Thanks for telling me your results. I hope it leads to additional gains in your other project.

BTW, it's not recommended to use the Rails constant in the application config. The recommended way is for you to put config.roadie.enabled = false in config/environments/test.rb.

Oh I put it in an initializer for roadie only (so I know it's for roadie)

I don't know what is the difference.
Could you tell me @_@?

OK the old project got more examples and more style
So just ignore the last one

Setup

  • config.roadie.enabled = true/false in test.rb
  • Run mailer spec only, with 95 examples (all with mail call)

Result

  • ON: Finished in 1 minute 5.78 seconds
  • OFF: Finished in 3 minutes 43.41 seconds

Comment

It's working!
(for turning it off in certain environment(s), not per request)

Owner

Mange commented Nov 1, 2012

Oh I put it in an initializer for roadie only (so I know it's for roadie)

I don't know what is the difference.
Could you tell me @_@?

Basically, initializers are always run while the different config/environments/*.rb are only loaded for their respected environments.
The point is to have all environment-specific configuration in one place so it's easy to reason about them, e.g. "the only difference between development and production is X, Y and Z."
Initializers are meant for setting up gems and other things that are always the same between environments.

It's working!

I'm glad too hear that! I hope you still left Roadie on in some integration tests, though. :-)


I'm still not closing this as @greggroth has another use case for this that requires more thought, detailed earlier in the conversation.

Thanks for your explanation :)

Hope you can release a new version soon
(so I can bundle update and mere the branch)

Owner

Mange commented Nov 6, 2012

Version 2.3.4 has just been released (with some additional fixes). Enjoy!

👍

👍 x2
Gonna update tomorrow 😄

Owner

Mange commented Jun 27, 2014

This is now handled by roadie-rails version 1.0.0. The Automatic module only inlines on delivery, and if you use deliver to show the message (letter_opener does this, for example) you can add the manual module Mailer instead and handle this however you want to.

@Mange Mange closed this Jun 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment