sighmon / mjml-rails

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

Refactor discovery of binary #66

Closed doits closed 3 years ago

doits commented 3 years ago

This is my try to make binary detection better and more error prone:

refactor detection of binary and unify executing mjml

- detect local first, then global
- use `yarn run` if installed with yarn, makes it work with yarn v2
- only use bin path when installed with npm, yarn should always work
  with `yarn run`
- always execute mjml in the same way, whether when checking version or
  transforming mail

This should fix #55.

Second commit:

always use full paths for binaries

Should fix #63

sighmon commented 3 years ago

@doits I don't use yarn, so would you mind adding some install/testing instructions?

doits commented 3 years ago

I updated the readme how to install it with yarn.

To install yarn, please see https://classic.yarnpkg.com/en/docs/install – note that you need yarn v1, because mjml itself is not compatible with yarn v2 yet. Then you can simply install mjml in your project with yarn add mjml and it should work.

Oh and you can then start your project (e.g. rails console) and look at Mjml::BIN to see if it is actually using yarn run.

sighmon commented 3 years ago

@doits Nice one, thanks. Looks good so far - only thing is one of the tests needs updating. I'm seeing:

Failure:
Mjml::Parser::#run::when shell command is failed#test_0001_raises exception [/Users/simos/Sites/heroku/mjml-rails/test/parser_test.rb:76]:
unexpected invocation: #<Mock:stderr>.success?()
unsatisfied expectations:
- expected exactly once, not yet invoked: #<Mock:stderr>.read(any_parameters)
- expected exactly once, not yet invoked: #<Mock:stderr>.eof?(any_parameters)
satisfied expectations:
- allowed any number of times, invoked once: Open3.popen3(any_parameters)

bin/rails test Users/simos/Sites/heroku/mjml-rails/test/parser_test.rb:75
doits commented 3 years ago

ahh right, forgot the tests! I fixed it be removing the stub (I think this is better than stubbing it). In addition I refactored the other tests to use newer minitest syntax and remove deprecation warnings. Oh and it looked like the tests for correct error messages weren't working at all, so I rewrote them, too.

Hope this is good now :-)

sighmon commented 3 years ago

@doits Brilliant work, thanks so much. I've merged and pushed 4.4.1 to Rubygems now.

chasegiunta commented 3 years ago

Any way this can support the binary in a different path other than root? Via a config setting, perhaps? All of our frontend dependencies live in /frontend, so it's not being discovered. I can create a separate issue for this.

doits commented 3 years ago

Try setting Mjml::BIN manually in an initializer, eg Mjml::BIN = "path/to/mjml"

I think it will still print the warning but should work. It will not check if the version of mjml is correct though.

doits commented 3 years ago

see #67 where I added a config option for it. If #67 is merged it is a lot easier, let's see what @sighmon says about it.