handle asset_host

[afeld]

Fixes #10. Fixes #11.

I was running into the same problem as #9, where I had stylesheets and images linked from my HTML emails. This PR takes a slightly different approach than the existing code, where it intercepts the asset_host as the email is being rendered, rather than using matchers to strip them after the fact, as was being discussed in #10, which is messy for non-trivial asset_host Procs.

Please let me know if there are edge cases that I didn't consider!

[afeld]

There may be a way to re-use the AssetUrlHelper mixin rather than copying-and-pasting, but I didn't want to spend too much time on it before getting initial feedback.

[Mange]

Yeah, this part needs improvement. I might be able to take a stab at it if you don't have any ideas.

My initial thought is to move all of this into a new class (so the replacement proc is as simple as possible, offloading everything to this new class) and have it include the helper. It would also make it easier to unit test.

[coveralls]

Coverage decreased (-0.8%) when pulling b46da40 on afeld:asset_host into 0c95273 on Mange:master.

[afeld]

The build failure seems to be a Rails 3.x compatibility problem – I'll fix later, if the overall approach seems ok.

[afeld]

P.P.S. This may be a tangential question, but what should the relationship of config.roadie.url_options and config.asset_host be? Seems like the former could be a default for the latter, within the mailers that have the roadie-rails mixin. Thoughts?

[Mange]

I would prefer not to use ActiveSupport::Concern, but I cannot find any rational reason not to (we're inside the Rails adapter, after all!) so I'll approve it.

[PikachuEXE]

Any bad side effects for using ActiveSupport::Concern?
I am curious (since I use in my gems as well)

[Mange]

It's just that standard Ruby handles this pretty easily itself without the extra sugar (which feels like adding sugar to your soda IMO, maybe it's a bit sweeter but I don't see the point right here).

---

Strawman time

It's a bit like having an ActiveSupport library that overrides the new method on classes like this:

module NewWithOptionalBlock
  def new(*)
    instance = super
    if block_given?
      yield instance
    end
    instance
  end
end

I just don't see the point in using an external library for this pattern when just adding it straight to your class would do. Just accept a block in initialize and be done with it.

---

But again, I cannot say a good reason not to do it here as we're in Rails, and the argument that it's so easy to implement without Rails means that if Rails ever removes it, it's very easy to remove again. It's a double-edged sword that way. :-)

[PikachuEXE]

I see. Thanks for your explanation.

[Mange]

P.P.S. This may be a tangential question, but what should the relationship of config.roadie.url_options and config.asset_host be? Seems like the former could be a default for the latter, within the mailers that have the roadie-rails mixin. Thoughts?

It's a tough question. On one hand, I'd like not to set an asset_host from it just to keep the Rails and Roadie parts touching as little as possible, but on the other hand, we'd leverage Rails' own helpers when generating image tags at least.

---

I think this PR has a good direction. I still feel a bit uneasy about the solution, even after sleeping on it for a bit, but it's also one of the cleanest solutions I can imagine right now.

What I'm thinking about most is:

• Is this solution robust enough?
  • By actually generating the correct URLs first, rather than "fixing" them after the fact, we should get a pretty robust solution here.
  • The current direction forces behavior on the mailer when the modules are included, which is a bit unstable.
• Will it cause incompatibilities down the road?
  • It's entirely possible that the asset helper and or asset_host changes in a future version of Rails, either in naming or semantics. It's definitely the most "unstable" part of roadie-rails right now.
  • By being based on imperative code when the module is included, it would be pretty easy to make some respond_to? calls before mutating anything.
• If I decide I want to change this later, how hard is it to change the API?
  • The API is hidden from the user right now. They don't interface with it, except for having an asset host set.
  • If I want to rip it out later, the module itself could be extracted from the gem and manually included by users requiring it.

All in all, it seems like this is going in. I want som extra polish first, though. Thank you for taking it this far! You don't owe me anything, so if you want to leave it here I can take it further myself. If you, however, feel up for it, I'll welcome additional changes from you.

Questions for you: What do you think about removing the automatic inclusion of the module and instead relying on either a "macro" or a manual inclusion of a module?

class SomeMailer
  include Roadie::Rails::Mailer
  include Roadie::Railer::AssetHostSupport
  # or:
  # roadie_mutate_asset_host
end

This requires a bit more setup, documentation and explanation, but it makes it opt-in and much easier to change around in the future, but it also removes part of the point of having a fully-integrated "rails gem".

It appears like you're using this fork in a project already. Is it running in production? Is it working properly?

[nateberkopec]

Thanks for all your work on this. asset_host compatibility is the one thing that keeps roadie-rails from 100% unadulterated awesome ATM.

EDIT: I'm going to try this branch on our app in prod, we'll see how it goes.

[Mange]

Thank you, @nateberkopec. 😄

[nateberkopec]

We've been using this on a Rails 4.X app for a while now and it's working as-expected. My asset_hosts are strings though, not procs, so we're probably the simplest use case :)

[Mange]

That's very nice to hear! Thank you. 🐗

[leonelgalan]

I'm gave the fork a try in my app and (obviously) I'm getting the same results: works well with the asset_host set, all images load and styles are inlined. Thanks @afeld for the PR and @Mange for Roadie!

I like the idea of explicitly including Roadie::Railer::AssetHostSupport 👍

EDIT:

I got this error locally:

Roadie::CssNotFound: Could not find stylesheet "/assets/mail-4e4790adbfa5cca77c240b866206601b360b3ba74ec8b469b1168b94fe0fa32b.css"
Used provider:
ProviderList: [
    #<Roadie::FilesystemProvider:0x007fb667f6b358>,
    #<Roadie::Rails::AssetPipelineProvider:0x007fb667f6b100>
]

[PikachuEXE]

Does it handle asset host with protocol relative URI?

[arashm]

Is there any plan to actually merge this?

[Mange]

Not at this time. I have other things that take my attention right now.
If someone steps up and take this to the finish line, I'd be glad to merge it. I don't want to merge this before it's finished.

[Mange]

Version 1.1.0.rc2 might make this easier to handle, even though it's still nowhere near automatic.

config.roadie.external_asset_providers = Roadie::PathRewriterProvider.new(config.roadie.asset_providers) do |url|
  if url =~ /myapp\.com/
    URI.parse(url).path.sub(%r{^/assets}, '')
  else
    url
  end
end

This will strip https://myapp.com/assets/ from any external URL in the email. Would this help?

I'm thinking about writing a default provider that strips away the asset host and adds it as a default provider if you have a asset host configured.

[seanlinsley]

I just merged this branch on my company's fork, because we needed #47: https://github.com/modernmsg/roadie-rails/commit/69b62dd09fb1d730734d1f14ce2986510645bf2e

@afeld if you're still interested in this PR, would you mind rebasing it?

[seanlinsley]

Note that even with this PR, we had to use a literal <link> to get Roadie to include styles from our Resque server, like suggested here: #42 (comment)

[arashm]

​We actually moved to premailer GEM due to this issue, and it's been working fine for us so far.

[Mange]

Just checking in here. Now that it's possible to handle assets from external URLs, one should be able to just use the normal asset_host, and then pick it up and rewrite it using an external_provider. If anyone manages to wire on up for their case and want to share it, this would be a perfect place to do so.

It's a bit crazy to think about all the pieces you need to fit together to make it work[1], so if we figure out a nice way to do it here, I might make it more first-class.

It's also possible to keep working on this to try to get everything working transparently out of the box, but I'm still worried about overriding private APIs of Rails and I'd like to avoid it as far as possible.

---

[1]: You'd need to add something like this as external provider:

def roadie_options
  options = super.dup
  # Always try this first of all
  options.external_providers.push(
    # Rewrite the URL to a local path, and delegate it to the local providers
    # Rewrites "https://cdn.myapp.com/assets/stylesheets/foo-1234.css" to "/stylesheets/foo-1234.css".
    Roadie::PathRewriterProvider.new(options.providers) do |url|
      # Make your own match here, that fits your case
      if url =~ /cdn\.myapp\.com/
        # Strip everything except for the path, then remove the `/assets` prefix
        URI.parse(url).path.sub(%r{^/assets}, '')
      else
        url
    end
  )
  options
end

[EDIT: I should add, you can add this to the global configuration too, without the need to override a method, dup, and push and so forth. Just assign the option.]

[tomasc]

@Mange I think that's a good solution – confirming it works:

def roadie_options
  options = super.dup
  options.external_asset_providers = [
    Roadie::PathRewriterProvider.new(options.asset_providers) do |url|
      url =~ Regexp.new(Regexp.escape(Settings.asset_host)) ? URI.parse(url).path : url
    end
  ]
  options
end