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

Fully support asset_host #11

Open
Mange opened this issue Jul 8, 2014 · 11 comments · May be fixed by #30
Open

Fully support asset_host #11

Mange opened this issue Jul 8, 2014 · 11 comments · May be fixed by #30
Milestone

Comments

@Mange
Copy link
Owner

Mange commented Jul 8, 2014

Here are my notes about this feature so people can contribute with ideas and feedback.

Current issues:

  1. Asset host is not applied to assets that get full URLs
  2. When asset host is set, stylesheet_link_tag becomes an absolute URL and inlining will not happen

1. Asset host is not applied

  • This is out of scope / wontfix
    • After all, why don't you just use image_tag, etc.
    • Can I find any examples in the wild where it makes sense?

2. Does not inline stylesheet_link_tags with asset_host

  • Not trivial to fix. asset_host can be String, Array or callable.
    • String is easy to fix; see if full URL starts with this string and strip it.
    • Array is also pretty easy. Do the String check for each item in the array.
    • callable is much harder. It could be doing anything, really.
      • User should provide a callable that reverses asset_host?
      • Take a guess (=~ %r{/assets/})? No. Error prone and too magic. Will break in all kinds of ways.
      • Take basename of file and look for it anyway? Will break for some people that want to be able to actually link to external stylesheets that happens to have the same name as a local asset too. ("Hey, this is why I used a full URL!")
      • Don't support this? How to communicate it? It will be seen as a "silent error" by most, and callable asset_host is a very common pattern.
        • If asset_host is set to a callable, require a reversing proc or fail with an error message.
          • But what about those who don't want it? They might just be after inline <style> elements or something. This requires them to work more.
          • This error will mostly only be detected in production. That's a very bad experience.
  • Will require changes to roadie
    • Roadie does not even ask asset providers on absolute URLs. AssetProviders need to provide a new API that can resolve absolute URLs.
      • asset_provider.find_using_url(url) if asset_provider.respond_to?(:find_using_url) perhaps? This does not require a major version bump; old implementations work like before.
  • Alternative solution (by Aidan Feldman) handle asset_host #30: Mutate asset_host to not do anything when encountering stylesheets.
    • This will use the normal asset host on anything that Roadie doesn't need
    • Stylesheets will still get relative URL so Roadie can pick them up.
@Mange Mange added this to the Version 1.1 milestone Jul 8, 2014
@afeld afeld linked a pull request Dec 29, 2014 that will close this issue
@nateberkopec
Copy link
Contributor

The fact that I have to set asset_host to nil is definitely annoying when you expect "image_tag", etc to work the way you expect as well :(

@nateberkopec
Copy link
Contributor

If I'm not mistaken, I don't think issue 1) is fixed by using image_tag.

image_tag "[email protected]"
http:///assets/[email protected]"

@Mange
Copy link
Owner Author

Mange commented Jan 15, 2015

I'm expressing myself really bad up there. The point is that I don't want to build in support for asset_host-like behavior inside roadie, e.g. that the hostname could be semi-random. I'd like to keep url_options as a static hash for simplicity.
If someone would want semi-random hostnames on images, they could generate the email as such. That was the point. (It's not really possible right now due to this bug, though. But when it's fixed, I won't add similar semantics.)

Is that clearer?

@maxwell
Copy link
Contributor

maxwell commented Jan 21, 2015

One thing I am a bit confused by... will Roadie options for host take the place of the asset host? Or can I only serve the inlined CSS directly from my application server? I guess it is inlined, so it would just be read off of disk?

Thanks for all your hard work here, I really appreciate Roadie!

@Mange
Copy link
Owner Author

Mange commented Jan 28, 2015

One thing I am a bit confused by... will Roadie options for host take the place of the asset host?

Roadie's URL options is only applied to relative paths. The reason this could be useful is when you're generating emails outside of a Rails request cycle (crontab, background job, etc.) and Rails does not know which hostname the user visited and cannot generate urls with the foo_url helpers without you supplying a hostname manually.

Roadie aims to make this easier to the user. Just configure the hostname that should be used in one place, and all the URLs will be changed. Keep on using foo_path, but get a foo_url "for free".

Since "Read more" links shouldn't go to asset hosts, the configured asset host is different from the Roadie URL options. Sure, relative paths inside CSS is also treated with the Roadie URL options to save you from trouble, but if you are among the asset host users, you already have a centralized config for where assets can be served from, and the full URL can be used in the email in the first place. Roadie isn't needed in that situation, so again, it does not need to use asset host.

tl;dr: If you have an asset host, Roadie does not need it. If you don't have an asset host, Roadie does not need it. :-)

Bullet 1 of this issue ("Asset host is not used") is out of scope / wontfix. Bullet 2 ("Cannot inline with an asset host") is the actual bug, being worked on in #30. I should make this clearer.

Or can I only serve the inlined CSS directly from my application server? I guess it is inlined, so it would just be read off of disk?

Yes, Roadie will not download stylesheets from the internet. You could write your own asset provider that does that, however[1]. You could even attach some caching layer if you wanted to.

If you have the stylesheets on the application server it will inline the contents, so you don't need to serve the assets anywhere.

Thanks for all your hard work here, I really appreciate Roadie!

That warms my heart. Thank you!

[1]: The inliner will only ask the provider about stylesheets with relative paths right now (someone needs to tell me if they need this).

@Mange
Copy link
Owner Author

Mange commented Sep 14, 2015

Yes, Roadie will not download stylesheets from the internet. You could write your own asset provider that does that, however[1]. You could even attach some caching layer if you wanted to.

Version 3.1 of Roadie can do this. It's still just a RC, but roadie-rails 1.1.0.rc2 onwards have support for it as well.

1.1 of roadie-rails will have better support, utilizing Rails cache and maybe even automatically handles the asset host differently.

@jufemaiz
Copy link

Hi @Mange , thanks for all your hard work!

Could you provide an update on the asset_host issue?

Cheers!

@Mange
Copy link
Owner Author

Mange commented Jun 29, 2016

I've recently started working on a Rails 3 app with an asset_host, so I
might be able to get some insight on this now.

I still feel the best way out of this is to manually build the URL for your
stylesheet in the email, or to add external providers that rewrites your
CDN resources to look like local ones. It's a bit more configuration than
getting it out of the box, but then again, this is a very tricky problem
that only applies to people working with non-default configs so some
configuration might be necessary.

I'm still extremely open to good suggestions on how to handle this
automatically.

Den tor 23 juni 2016 01:15Joel Courtney [email protected] skrev:

Hi @Mange https://github.com/Mange , thanks for all your hard work!

Could you provide an update on the asset_host issue?

Cheers!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAAGP6a2IHqGFzdsLndHiBQgc2jR515wks5qOcIIgaJpZM4CLIsL
.

@jufemaiz
Copy link

Hi @Mange, thanks for the feedback! Good to know what the opinion is of the author :)

I've managed to get it working for the time being using a workaround posted at #10 (comment)

I think that you're almost there. I disagree that hand crafting is a good thing – but I'm also understanding that as the author you have valid reasons for your opinion, particularly with the underlying Roadie Gem being a non-rails implementation.

Let me do some more thinking and document some more use cases where I believe there is a need (predominantly around image versus stylesheet collision issues).

Cheers for all your hard work – it's very much appreciated!

@Mange
Copy link
Owner Author

Mange commented Jun 30, 2016

Thank you for your kind words. I look forward to hearing about your
findings. ☺️

Den tor 30 juni 2016 00:13Joel Courtney [email protected] skrev:

Hi @Mange https://github.com/Mange, thanks for the feedback! Good to
know what the opinion is of the author :)

I've managed to get it working for the time being using a workaround
posted at #10 (comment)
#10 (comment)

I think that you're almost there. I disagree that hand crafting is a good
thing – but I'm also understanding that as the author you have valid
reasons for your opinion, particularly with the underlying Roadie Gem being
a non-rails implementation.

Let me do some more thinking and document some more use cases where I
believe there is a need (predominantly around image versus stylesheet
collision issues).

Cheers for all your hard work – it's very much appreciated!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAAGP9OVeXJApVAACkdHrnAbamW0GjQbks5qQu4HgaJpZM4CLIsL
.

@glebm
Copy link
Contributor

glebm commented Mar 26, 2017

When asset_host is used for images, the stylesheet URL can still be generated without the host like this:

<%# Ensure the stylesheet is referenced without a host so that the local
    version is read by Roadie even if asset_host is set. %>
<link rel="stylesheet" href="<%= stylesheet_path('email', host: '') %>" />

This seems like a better workaround than setting asset_host to nil as the readme suggests.

glebm added a commit to thredded/thredded_create_app that referenced this issue Mar 26, 2017
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 a pull request may close this issue.

5 participants