search

superbigco / craft-mjml

Render Twig emails with MJML.
https://superbig.co
MIT License
25 stars 5 forks source link

Regression in MJML binary usage with npx, nvm #26

Closed soulseekah closed 4 months ago

soulseekah commented 4 months ago

This commit https://github.com/superbigco/craft-mjml/commit/00db9a9048d3ca9d0c3bb024b4be7ab917541a40 introduces a regression and a breaking change in how mjml can be (and is) used with npx without a global installation, and also introduces friction in nvm-based environments.

We used to be able to just supply the following configuration:

MJML_NODE_PATH=npx
MJML_CLI_PATH=mjml

This now throws an exception. And forces us to:

  1. Supply the full path to a node binary, which under nvm is based on the current user directory and node version installed, which will change with node updates and across users (for example: /home/$HOME/.nvm/versions/node/v$VERSION/bin/node).
  2. Install mjml globally or under nvm, or supply the node_modules path, which also changes (see 1.)

This severely limits a flexible setup with non-globally installation of mjml and node, and breaks usage via npx altogether.

Broken via https://github.com/superbigco/craft-mjml/issues/20

Code that breaks with file_exists https://github.com/superbigco/craft-mjml/commit/00db9a9048d3ca9d0c3bb024b4be7ab917541a40#diff-1639e3c1b104889ebfbeb509353e2334602ecad5d689941359b4166f2ff6720cR147-R156.

There are several approaches in fixing this and I'm available to help solve this.

One of the easier ways, I believe, is to check all PATH= environment variables that are passed onto PHP to find the binaries. If npx is the binary, the mjml path should not be checked with file_exists.

linear[bot] commented 4 months ago

PLU-158 Regression in MJML binary usage with npx, nvm

Romanavr commented 4 months ago

Just encountered the same issue. Please fix ASAP⚡

sjelfull commented 4 months ago

@soulseekah Ah, thanks for opening this. Of course this broke npx and I assume pnpm dlx as well. Would you mind opening a PR? I'm heading on vacation next week and won't have much time in a while for this.

soulseekah commented 4 months ago

@soulseekah Ah, thanks for opening this. Of course this broke npx and I assume pnpm dlx as well. Would you mind opening a PR? I'm heading on vacation next week and won't have much time in a while for this.

@sjelfull thanks for your support. PR is opened, hopefully we can polish and fine-tune the approach before your vacation and have a minor release ;)

vadimgodev commented 4 months ago

Waiting for the PR, need it asap

sjelfull commented 4 months ago

Thanks, new release tagged!

daniellelecomte commented 1 month ago

@sjelfull - can this fix also be rolled into the Craft 4 version?