search

sighmon / mjml-rails

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

Add faster alternative for checking mjml version #127

Closed danieldiekmeier closed 1 week ago

danieldiekmeier commented 1 week ago

Fixes #125.

As I suggested in #125, I assume most people that use Rails and Mjml together probably install Mjml via yarn, and the binary is probably in the node_modules folder. If it's in there, we can look for the version number directly in the package's package.json, and don't have to actually run it.

The benefit is that this way of checking the version is much, much faster.

If this "happy path" check is unsuccessful, it will fall back to the "classic" version.

danieldiekmeier commented 1 week ago

The CI failures are due to the Haml class now breaking the "Metrics/ModuleLength: Module has too many lines. [105/100]" lint rule. One could probably extract a BinaryFinder class with all the ways to search binaries, but I didn't want to do that as a drive by refactor in this PR. What can I do?

sighmon commented 1 week ago

@danieldiekmeier I'm very happy for you to extract it into a BinaryFinder class if you have the time/energy.

danieldiekmeier commented 1 week ago

I looked into it, but with the current internals, this feels like too much work to refactor in a way that keeps the current behaviour the same. (e.g. that Mjml.mjml_binary_error_string is a mattr_accessor and can be set by users, but can also be changed by the MRML finder. Or all the ways that deprecated options are being upgraded on the fly.)

To be honest, we're planning to move away from MJML over time, and until then, I'll just live with my monkey patch from #125.

sighmon commented 1 week ago

@danieldiekmeier Thanks for looking into it - just out of interest, what are you moving to?