-
Notifications
You must be signed in to change notification settings - Fork 26
feat: Option to search sass executable #94
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
base: main
Are you sure you want to change the base?
Conversation
bocharsky-bw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think we don't need that extra arguemnt, how about simplify the business logic in that method?
| $binaryPath = $this->binaryPath; | ||
|
|
||
| if (null === $binaryPath && $this->searchBinary) { | ||
| $binaryPath = (new ExecutableFinder())->find('sass'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we need this searchBinary extra parameter? Why we can't just tweak this logic to:
| $binaryPath = $this->binaryPath; | |
| if (null === $binaryPath && $this->searchBinary) { | |
| $binaryPath = (new ExecutableFinder())->find('sass'); | |
| } | |
| $binaryPath = $this->binaryPath; | |
| if (null === $binaryPath) { | |
| $binaryPath = (new ExecutableFinder())->find('sass'); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I need it because in my case $this->binaryPath is null so $binaryPath will be null and an executable will be found.
I want to force $binaryPath to be null here, thanks to that SassBinary will download the binary locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that's what I'm talking about... if you have binaryPath set to null, then this code should be enough for you to find the binary:
$binaryPath = $this->binaryPath;
if (null === $binaryPath) {
$binaryPath = (new ExecutableFinder())->find('sass');
}Or you don't want the bundle to use your global sass locally and always download the new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you don't want the bundle to use your global sass locally and always download the new one?
yes that's it! :)
I recently encountered this issue (see #84) during a deployment to a cloud environment. Since this specific environment restricts the installation of external binaries, I suggest adding a configuration option that allows the bundle to automatically download the appropriate binary during cloud build phase.