Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the developer to specify a path to the node executable. Fixes #6. #7

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

arvcork
Copy link
Contributor

@arvcork arvcork commented Oct 29, 2023

I was facing an issue where the ExecutableFinder could not resolve the path to my homebrew node installation.

Performing which node revealed that I was using homebrew. Hardcoding this string into the package allowed the executable to run and the email to be sent.

@arvcork arvcork mentioned this pull request Oct 29, 2023
@maantje
Copy link
Owner

maantje commented Nov 1, 2023

Hey @arvcork 👋,

Thanks for your contribution! I'll merge it, but the tests are currently failing. The issue lies in this line. The default is never passed since the configuration is defined in react-email.php with null.

To fix it, consider changing it to:

config('react-email.node_path') ?? app(ExecutableFinder::class)->find('node')

This should resolve the issue.

@lvillacin
Copy link

Hi there! I hope I'm not being pushy but was wondering if we got an update on this? @arvcork ? Thank you very much. Currently will be deploying in a few days and was hoping to use your patch when I do so. Much thanks!

@arvcork
Copy link
Contributor Author

arvcork commented Nov 18, 2023

Hi all, apologies about this. I didn't get the notification for this!

I'll work on the fix today based on what you have said @maantje and get the test passing and then we can look to merge potentially.

@maantje maantje merged commit e79a54d into maantje:main Nov 21, 2023
4 checks passed
@maantje
Copy link
Owner

maantje commented Nov 21, 2023

Hey @arvcork 👋,

Thanks again for your contribution. I merged the pull request and created a new release v0.3.0.

@lvillacin
Copy link

Thank you very much @arvcork and @maantje!

@lvillacin lvillacin mentioned this pull request May 2, 2024
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.

3 participants