Missing --load-path option

[justin-oh]

In the documentation for integrating Bootstrap, it is recommended to composer require twbs/bootstrap then @import '../../vendor/twbs/bootstrap/scss/bootstrap'. It becomes cumbersome, tedious, ugly, etc. to maintain relative pathing like that. If you have to move a file, you must update these paths.

To make matters worse, I have my own bundle with a Resources/public directory that copies some Sass into the public directory of the Symfony project. So now I have to think like this:

/* vendor/org/style-bundle/src/Resources/public/scss/style.scss */

// this file (style.scss) will be copied to /public/bundles/orgstyle/scss/style.scss
// so we need to reference the Bootstrap vendor files from that location
@import '../../../../vendor/twbs/bootstrap/scss/bootstrap';

I could of course not put the Sass file in the public directory, but my import would still look ugly:

/* vendor/org/style-bundle/src/Resources/scss/style.scss */

@import '../../../../../twbs/bootstrap/scss/bootstrap';

I don't think the --load-path option would necessarily need to be exposed to the developer, but it would be nice if the bundle provided some basic/common load paths like this:

--load-path=/absolute/path/to/project/node_modules --load-path=/absolute/path/to/project/vendor

Then we can at least have more readable/maintainable Sass imports for 3rd party code:

@import 'twbs/bootstrap/scss/bootstrap';

[bocharsky-bw]

Hey @justin-oh ,

I suppose some IDEs like PhpStorm may refactor those import paths for you when you move files in a different dir. Kinda of an edge case, I also don't think we strongly need this because mostly you don't move files around so that long/ugly path is written only once. I wonder if someone else is experiencing similar problems in their project.

But I'm not against this --load-path feature if that would help some devs, if you want to give it a try and open a PR - feel free to do it, PRs are warmly welcome

[justin-oh]

I'm unsure what the accepted approach would be.

Simple Approach

If we assume Sass files would only be loaded from the /assets, /node_modules, and /vendor directories of the project, the approach is simple. In Symfonycasts\SassBundle\SassBuilder::getBuildOptions(), prior to returning, add the following:

$buildOptions[] = '--load-path='.escapeshellarg($this->projectRootDir.'/node_modules');
$buildOptions[] = '--load-path='.escapeshellarg($this->projectRootDir.'/vendor');

It Gets Complicated

If the need is to actually support a configurable --load-path for the developer, the approach gets a bit complicated. To fully support said argument, you need to consider an array because there can be many --load-path arguments.

However, the SassBuilder's current options are all assumed to be simple values like strings and booleans. Array consideration would need to be added.

Further, the config keys are expected to essentially match what is offered by Sass. It would be preferable to name the new config value load_paths to indicate it is an array, but the getOptionMap function would map that value to --load-paths which is not a valid Sass command line argument.

Overhaul Needed?

Upon reviewing the way those options are handled, I think they should be overhauled so they aren't passed into the SassBuilder as an array parameter with a corresponding map, but handled through various helper functions specific to certain options. Currently, everything is being handled in a loop which starts to get out of hand quite easily. There are str_starts_with checks and is_string checks. To implement --load-path there would need to be is_array checks.

Here is my proposed approach where the SassBuilder class has several properties that are specific to certain arguments. There is no loop. There is just a series of checks to determine if an option/argument needs to be added as the command is being built:

class SassBuilder
{
    // these are the only values Sass allows for --style
    const STYLE_COMPRESSED = 'compressed';
    const STYLE_EXPANDED = 'expanded';

    // this is the default as defined by Sass
    private string $style = self::STYLE_EXPANDED;

    // this covers the --charset and --no-charset arguments
    // which should never exist together, so it makes sense to control them with a boolean variable
    private boolean $charset = false;

    // etc.

    public function __construct(
        private readonly array $sassPaths,
        private readonly string $cssPath,
        private readonly string $projectRootDir,
        private readonly ?string $binaryPath,
    ) {
    }

    // with these 2 functions, there is no way to set a value that Sass will be angry about
    public function setStyleCompressed(): self
    {
        $this->style = self::STYLE_COMPRESSED;

        return $this;
    }

    public function setStyleExpanded(): self
    {
        $this->style = self::STYLE_EXPANDED;

        return $this;
    }

    // however, if the above 2 functions makes implementation difficult
    // then it is easy enough to have the following and rely on proper usage
    public function setStyle(string $style): self
    {
        $this->style = $style;

        return $this;
    }

    public function setCharset(bool $charset): self
    {
        $this->charset = $charset;

        return $this;
    }

    // etc.

    // building out the command based on the helper properties
    // such that there is no way to have arguments that contradict each other
    public function buildCommand()
    {
        $options = [];

        $options[] = '--style='.$this->style;

        if ($this->charset) {
            $options[] = '--charset';
        } else {
            // this is redundant by Sass documentation as it is the default behaviour
            $options[] = '--no-charset';
        }

        // etc.
    }
}

To complete what is currently implemented means adding a bunch of boolean properties like charset to correspond with the remaining options (ie. --error-css, --no-error-css, --source-map, --no-source-map, etc.).

To integrate --load-path:

private array $loadPaths = [];

public function __construct(
        private readonly array $sassPaths,
        private readonly string $cssPath,
        private readonly string $projectRootDir,
        private readonly ?string $binaryPath,
    ) {
        // possibly add node_modules and vendor here as convenient defaults
        $this->loadPaths[] = $projectRootDir.'/node_modules';
        $this->loadPaths[] = $projectRootDir.'/vendor';
    }

public function addLoadPath(string $loadPath): self
{
    $this->loadPaths[] = $loadPath;
}

public function buildCommand()
{
    // ...

    // possibly apply array_unique to $this->loadPaths

    foreach ($this->loadPaths as $loadPath) {
        $options[] = '--load-path='.escapeshellarg($loadPath);
    }
}