mjml-rails
mjml-rails copied to clipboard
sighmon
Changing binary version does not work
Changing the binary version in a Rails initializer like...
Mjml.setup do |config|
config.mjml_binary_version_supported = '4.2.'
end
...does not work. The initializer is loaded too late. Mjml::BIN is computed earlier. I was able to override it by doing the following in my initializer after setup:
Mjml::BIN = Mjml.discover_mjml_bin
However, the error messages remain when loading the app.
Thank you for this. My production app kept breaking because of not finding the binary
Thanks for this workaround. I made a PR (#40) to address this problem.
Should there be more logic in setting the binary version to support different mjml version at the same time? Or is it ok for users to fix the mjml-rails gem version, if they want to use an older binary?
I suppose I've been wondering why there is a version check in the first place. Is there a technical reason for this? Shouldn't it be up to each developer to choose their version? Maybe the version check should be less restrictive, i.e. "4." instead of "4.1."?
Yes, I also think it should be a valid option to use the latest gem version but addressing an older binary for example.
@pbougie The reason for the version check is that there was a syntax breaking update between 4.0. and 4.1. but now that we're another point release up, I think the less restrictive check sounds fair.
@donni106 Thanks for the PR - I think we might set @@mjml_binary_version_supported = "4." - so I'll close your pull request, but please do submit another in the future. :-)
I'll push out an update shortly.
@pbougie @donni106 does this look okay to you both? https://github.com/sighmon/mjml-rails/commit/d9f8ce38d2a2e5a3bb57ab109e959a597c316fee
@sighmon The less restrictive version check sounds good.
Unfortunately, the Mjml::BIN = Mjml.discover_mjml_bin fix was meant to be temporary to keep my app running but it constantly emits errors (although I will be able to remove it with the less restrictive version check but should probably be refactored for the future):
Couldn't find the MJML 4.1. binary.. have you run $ npm install mjml?
./config/initializers/mjml.rb:6: warning: already initialized constant Mjml::BIN
./.bundle/gems/ruby/2.5.0/gems/mjml-rails-4.2.2/lib/mjml.rb:35: warning: previous definition of BIN was here
@pbougie I noticed those warnings, but wasn't sure how best to refactor it. Do you have any suggestions?
@sighmon Get rid of the BIN constant. What about changing Mjml::Parser.mjml_bin to Mjml.discover_mjml_bin? This way the path is lazy loaded. I think this will fix the Rails initializer configutation for mjml_binary_version_supported as well.
@pbougie I quite like that the BIN is a constant from a security perspective, in that it'll warn you if any other initialiser tries to change the binary being run.
I was trying to think of a long-term use case for specifying a different binary too - perhaps the persistent warnings are a good thing?
@sighmon Perhaps now that only the major version is required. However, if that's the case then the mjml_binary_version_supported configuration option should be removed because it doesn't work.
Although I'm still not sure there should be a version check altogether. Shouldn't it be up to the user to use the version they want? I found myself with a major bug earlier this week where I simply could not install MJML 4.1 using Yarn. No matter what I did, it installed MJML 4.1.2 but all the dependencies like MJML-* would only install 4.2.0. MJML-Rails gets the version number from MJML-Core I believe which reported 4.2.0. I could not get MJML-Rails to run without the hack that started this thread. Perhaps a simple warning when running MJML-Rails outside a prescribed version range is all that is required.
No matter what I did, it installed MJML 4.1.2 but all the dependencies like MJML-* would only install 4.2.0.
This is the same reason I came here. The mjml package 4.1.2 has all the dependencies with ^4.1.2 which updated minor release and not only patch versions. If it had be ~4.1.2 the problem with updating to 4.2.0 would never been raised. Maybe we have to report this to the package owner?
Otherwise as mjml-rails is not an official gem by mjml, they are not responsible for such issues.
@pbougie A warning sounds like a good way to go.
@donni106 I'm not quite sure I'm following you here:
The mjml package 4.1.2 has all the dependencies with ^4.1.2 which updated minor release and not only patch versions. If it had be ~4.1.2 the problem with updating to 4.2.0 would never been raised. Maybe we have to report this to the package owner?
When I try and install an older version of MJML with npm install -g [email protected] it says it has, but then mjml --version gives 4.2.0. Yet when I check /usr/local/lib/node_modules/mjml/package.json it lists the dependencies as ^4.1.2 correctly. So is this the bug that you're wanting to report?
When I try and install an older version of MJML with npm install -g [email protected] it says it has, but then mjml --version gives 4.2.0
Yes, that was strange for me. Shouldn't it be fix 4.1.2 for dependencies, when there is a minor update released? I know, this is nothing related to the gem here.
I'm curious about this thread. Yesterday after pushing out an update that include an update in package.json from node ^10.9.0 to node ^11.1.0, ALL Mjml emails stopped rendering so I blank emails were going out. Reading this thread led me to rollback to the older version of node as well as use a previous yarn.lock file to get emails rendering again.
Not sure why going from MJML 4.1.2 to 4.2.0 is leading to blank emails though.
I ended up adding config.raise_render_exception = true to my mjml.rb initializer and that revealed
Mjml::Parser::ParseError: MJML v3 syntax detected, migrating to MJML v4 syntax. Use mjml -m to get the migrated MJML.
So upon further struggle bussing, I removed the <mj-container> which was wrapped around <%= yield %> and the emails started rendering again. It was the update from 4.1.2 to 4.2.1 for me that revealed this. I will have to read through the docs and see why this was an issue.
I was trying to think of a long-term use case for specifying a different binary too
Just an idea: the CLI version of mjml doesn't support custom components yet, so using a different binary can be a way to load custom components.
May I check, is there any clearer solution to resolving this issue? It would be great to clean up the warnings mentioned above.
@pmackay Could you make a tiny app to show how you're hitting that? It's been a long time since this thread. :-)
I think this issue fixed in the latest versions, since the Mjml::BIN constant is gone and the version check happens only after initialization. Changing the binary is possible now with:
# config/initializers/mjml.rb
Mjml.setup do |config|
config.mjml_binary = "/path/to/custom/mjml"
end
In addition errors are raised by default so you shouldn't get blank emails if something breaks after an upgrade, unless you manually disabled exceptions or set the validation level to soft.
