billhorsman / inline_styles_mailer

Inline Styles Mailer Gem
MIT License
14 stars 11 forks source link

Add Rails 6 Support #15

Open arlando opened 4 years ago

arlando commented 4 years ago

Hi @billhorsman

I am working on having this library support Rails 6. I ran into a few regressions with the inline-styles-mailer gem against Rails 6. I've tried to add support while maintaining backwards compatibility as much as possible.

In Rails 6 the _layout method's arity has changed. The render_to_string method when used with a :file argument now requirements an absolute path to the file.

On another note I believe the sass-ruby library has been deprecated and upgrading to higher versions of sass-rails now depends on sassc-ruby library see: https://github.com/rails/sass-rails/releases/tag/v6.0.0. This library changes the name space of Sass to SassC and there may not be sass feature rendering parity with previous Sass version see: http://sass-compatibility.github.io/. Hence, I made a lock of the sass-rails version to ~> 5.0.8 to avoid potential breaking changes. I'd recommended doing a version bump when upgrading that just to be extra cautious.

billhorsman commented 4 years ago

Hey @arlando

Thanks for the contribution 👏 I like the look of this. Let me get my head round the compatability issues with Rails < 6 and give you a better response as soon as I can.

billhorsman commented 4 years ago

@arlando I played around with this a bit at the weekend but ran out of time. It might be that to support Rails 6 we have to drop support for some earlier versions of Rails and Ruby. It would be good to mimimise that as much as possible. Worst case, we have a new major version of this gem that only supports Rails 6 but I'd like to avoid that if possible.

arlando commented 4 years ago

@billhorsman I'd like to avoid that too.

Rails 6 changed the arity of the _layout method. If we follow the pattern here we should be able to maintain backwards compatibility.

In Rails 6 the sass parser has changed. sass_rails is using sassc-ruby which changed the reference of Sass to SassC. If we can dynamically detect if inline_styles_mailer should use Sass or SassC here, I think we should be able to maintain support for previous versions of Rails. But, because the sass library changed there could be regressions.

ruby wise, Rails 6 depends on versions 2.5.0 of ruby and above. I wasn't able to find any regressions between 2.2.2 (Rails 5 base line support) and 2.5.0 ruby versions. Do you think there would be some code in inline_styles_mailer that would no longer work in older or newer versions of ruby? I wasn't able to detect this.

billhorsman commented 4 years ago

@arlando I think your arity checks are fine, it's just the gem dependencies that worry me — as can be seen in the Travis test results.

arlando commented 4 years ago

@billhorsman I'm new to this :smile: so there may be a smarter way of handling this. I will look into this more soon.

billhorsman commented 4 years ago

@arlando I'm not new to this but that doesn't make me smarter :) The most recent test results are here.