search

sighmon / mjml-rails

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

Fix discovering mjml binary when npm is missing #54

Closed NobodysNightmare closed 5 years ago

NobodysNightmare commented 5 years ago

This pull requests fixes discovery of the mjml binary, when there is only yarn (but no npm) installed.

In case a binary called through backticks is missing, ruby will raise Errno::ENOENT.

Thus the previous code failed to discover the bin path, when npm was not present on the system, because an error was raised immediately.

Reproduction

On a system that has yarn installed, but no npm (e.g. in a docker image) try booting your Rails application. Prior this PR, it should error out:

$ be rake -T
rake aborted!
Errno::ENOENT: No such file or directory - npm
/home/jan/development/hub/myapp/config/application.rb:20:in `<top (required)>'
/home/jan/development/hub/myapp/Rakefile:3:in `require'
/home/jan/development/hub/myapp/Rakefile:3:in `<top (required)>'
sighmon commented 5 years ago

@NobodysNightmare Have you looked into why the build is failing? https://travis-ci.org/sighmon/mjml-rails/jobs/571202562

NobodysNightmare commented 5 years ago

Whoops, I didn't notice. Specs are passing locally for me in these cases:

The only way they fail for me is when none of them is installed (since I don't have a global installation of mjml). I just fixed an error for the latter case, where I initially forgot to check whether we found an installer_path.

I still can't explain the CI failure though, but it seems clearly related to my changes :(

NobodysNightmare commented 5 years ago

I did a bunch of printf debugging to no avail.

My last attempt was reverting my changes: https://travis-ci.org/sighmon/mjml-rails/jobs/572624305

This run is essentially identical to the master branch, but failed as well... So I suppose something has happened to the CI pipeline in general, that made the build fail?

NobodysNightmare commented 5 years ago

I finally figured out what the problem was. Essentially the same thing happened to us as well: One of the dependencies of mjml now requires a more recent version of nodejs.

By letting nvm install node 10 the CI turns green.

NobodysNightmare commented 5 years ago

Hey @sighmon, is there any update from your side on this?

(Trying to use a mention here, in case you just "missed" my updates because of notification settings... if you are just busy: sorry for the additional mentioning)

sighmon commented 5 years ago

@NobodysNightmare Thanks so much for the update - looking through it slipped off my list, but found some time to look through it tonight. I've merged and pushed it now.