Skip to content

Support for JS Package Manager updates#107

Merged
sighmon merged 2 commits intosighmon:masterfrom
yuta-hayashi:fix-for-package-manager-updates
May 12, 2023
Merged

Support for JS Package Manager updates#107
sighmon merged 2 commits intosighmon:masterfrom
yuta-hayashi:fix-for-package-manager-updates

Conversation

@yuta-hayashi
Copy link

Fix: #104

As explained in #104, the behavior of yarn bin has changed since yarn 2. Also, the npm bin command has been removed since npm 9.
This PR has been modified to support the new versions of yarn and npm as follows.

For yarn, the yarn bin mjml command outputs the full path to the mjml binary, so use that.

However, npm does not have such a command, and npx and npm exec have more overhead than executing the binary directly.

So I used the npm root command to get the full path of the local node_modules and appended the .bin directory. This is the same path as the result of executing npm bin.
Also, the npm root command works with npm 6~9.
Refs:
https://docs.npmjs.com/cli/v9/commands/npm-root
https://docs.npmjs.com/cli/v9/configuring-npm/folders?v=true#executables

And I removed the bin_path_from method that was common to yarn and npm because of the difference in execution commands and processing.

It works correctly in my Rails Project using npm9, and I have verified it with yarn 2.

@yuta-hayashi yuta-hayashi force-pushed the fix-for-package-manager-updates branch from 32fea54 to 301ad79 Compare May 4, 2023 13:08
@yuta-hayashi
Copy link
Author

@sighmon
Hi, could you review this PR?

@sighmon
Copy link
Owner

sighmon commented May 11, 2023

@yuta-hayashi Sorry for the delay - I'll have time this weekend to look through it.

@sighmon sighmon self-requested a review May 12, 2023 06:44
Copy link
Owner

@sighmon sighmon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me thanks @yuta-hayashi

@sighmon sighmon merged commit 301ad79 into sighmon:master May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants