search

sighmon / mjml-rails

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

[Bug] Does not find MJML executable with npm 9+ (yarn 2+ uses npm discovery) #104

Closed keithlayne closed 1 year ago

keithlayne commented 1 year ago

I tried updating to node 18 (current LTS) which (at least with asdf) ships with npm 9.5.0. NPM 9 removed npm bin https://github.blog/changelog/2022-10-24-npm-v9-0-0-released/.

I just looked at the discovery code and realized that the yarn discovery never worked for yarn 2+ because yarn bin didn't return a path, but a list of executables. Therefore, npm was of course installed, so the npm lookup worked via npm bin because I was using node_modules with yarn.

A pretty simple fix for AFAIK all yarn users, regardless of version, would be to execute yarn bin mjml - this should find the full path to mjml (this would be a really simple PR).

Not sure what to do about npm 9+, in terms of a general solution.

For now, an initializer with this in it:

config.mjml_binary = Rails.root.join('node_modules/mjml/bin/mjml')

...solves the issue for me.

sighmon commented 1 year ago

@keithlayne Thanks for finding that bug - looks like the last GitHub Action ran with node 18 and npm 8.19.3 https://github.com/sighmon/mjml-rails/actions/runs/3965640687/jobs/6795562473#step:4:11

Would you like to create a PR with your solution that has a new matrix of npm versions to test?

Might also be worth adding your initialiser solution to the README.

keithlayne commented 1 year ago

Sorry, I meant to look at this over the weekend, but I just had an idea, which seemed to work. This is probably more in the readme category, but I tried:

Mjml.setup do |config|
  config.mjml_binary = 'yarn mjml'
end

...and this worked (with yarn 3). Bonus here is that it ought to work with yarn if you're not using node_modules (I'm not sure what yarn bin does in that situation).

Discovery seems kinda hard and full of weird corner cases. It probably still has value but a few lines of "with npm/yarn/pnpm/whatever lands next week, use this in an initializer:" in the readme might help a lot.