sighmon / mjml-rails

MJML + ERb/Haml/Slim view template
https://mjml.io
Other
295 stars 64 forks source link

Allow using MRML via MRML-ruby #114

Closed pokonski closed 7 months ago

pokonski commented 8 months ago

This proof of concept change would allow using Rust implementation of MJML called MRML via the mrml-ruby gem.

The benefits:

Would anyone except me find this useful?

References:

MRML isn't 100% compatible with MJML, so I made it optional.

sighmon commented 8 months ago

@pokonski The speed/lower memory use does look amazing. What's your feeling about the compromise of including mrml-ruby binaries when the default is not to use them? Should this be a fork?

If not, it'd be great if you could update the README and write a test when using the use_mrml flag.

I've also fixed the GitHub Action now for Ruby 2.7, so if you merge from master it should all work again.

pokonski commented 8 months ago

What's your feeling about the compromise of including mrml-ruby binaries when the default is not to use them? Should this be a fork?

Good question, I am not opposed but ultimately I can make it a "peer" dependency and check if it's present inside the parser with if defined?(MRML) and raise error in case it is not loaded. MRML makes the whole library simpler, but fragmenting the userbase of mjml-rails with frivolous forking wouldn't be my first choice 😁

If not, it'd be great if you could update the README and write a test when using the use_mrml flag.

Will do, I didn't focus on tests initially because I wasn't sure if such addition is desired :)

manuelmeurer commented 7 months ago

Would be great to see this implemented! 😄

pokonski commented 7 months ago

@sighmon I updated the documentation and made mrml a "peer" dependency. Will add some tests too :)

pokonski commented 7 months ago

I also added tests for MrmlParser, CI is now green in my fork:

https://github.com/pokonski/mjml-rails/actions/runs/7488219746/job/20382217087

let me know if anything else is missing :)

sighmon commented 7 months ago

That works for me! Thanks so much - I'll create a release today.

pokonski commented 7 months ago

Awesome, thank you!