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

Add binary version configuration #42

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions config/services.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
abstract_arg('path to css directory'),
param('kernel.project_dir'),
abstract_arg('path to binary'),
abstract_arg('binary version'),
abstract_arg('sass options'),
])

Expand Down
9 changes: 9 additions & 0 deletions doc/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,15 @@ You can configure most of the `Dart Sass CLI options <https://sass-lang.com/docu
# trace:


Using a different version
--------------------------
This bundle installs for you the latest version. However, if you want an explicit version of Dart Sass you can instruct the bundle to download that version, set the ``binary_version`` option:

.. code-block:: yaml

symfonycasts_sass:
binary_version: 1.69.0

Using a different binary
--------------------------

Expand Down
7 changes: 6 additions & 1 deletion src/DependencyInjection/SymfonycastsSassExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ public function load(array $configs, ContainerBuilder $container): void
->replaceArgument(0, $config['root_sass'])
->replaceArgument(1, '%kernel.project_dir%/var/sass')
->replaceArgument(3, $config['binary'])
->replaceArgument(4, $config['sass_options'])
->replaceArgument(4, $config['binary_version'])
->replaceArgument(5, $config['sass_options'])
;

$container->findDefinition('sass.css_asset_compiler')
Expand Down Expand Up @@ -87,6 +88,10 @@ public function getConfigTreeBuilder(): TreeBuilder
->info('The Sass binary to use')
->defaultNull()
->end()
->scalarNode('binary_version')
->info('The Sass binary version to download - null means the latest version')
->defaultNull()
bocharsky-bw marked this conversation as resolved.
Show resolved Hide resolved
->end()
->arrayNode('sass_options')
->addDefaultsIfNotSet()
->children()
Expand Down
47 changes: 37 additions & 10 deletions src/SassBinary.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@

class SassBinary
{
private const VERSION = '1.69.7';
private HttpClientInterface $httpClient;
private ?string $cachedVersion = null;

public function __construct(
private string $binaryDownloadDir,
private ?string $binaryPath = null,
private ?string $binaryVersion = null,
bocharsky-bw marked this conversation as resolved.
Show resolved Hide resolved
private ?SymfonyStyle $output = null,
HttpClientInterface $httpClient = null
) {
Expand All @@ -34,7 +35,7 @@ public function __construct(
public function createProcess(array $args): Process
{
if (null === $this->binaryPath) {
$binary = $this->getDefaultBinaryPath();
$binary = $this->getDefaultBinaryPath($this->getVersion());
if (!is_file($binary)) {
$this->downloadExecutable();
}
Expand All @@ -49,7 +50,7 @@ public function createProcess(array $args): Process

public function downloadExecutable(): void
{
$url = sprintf('https://github.com/sass/dart-sass/releases/download/%s/%s', self::VERSION, $this->getBinaryName());
$url = sprintf('https://github.com/sass/dart-sass/releases/download/%s/%s', $this->getVersion(), $this->getBinaryName());
$isZip = str_ends_with($url, '.zip');

$this->output?->note('Downloading Sass binary from '.$url);
Expand All @@ -75,6 +76,13 @@ public function downloadExecutable(): void
},
]);

if (404 === $response->getStatusCode()) {
if ($this->getLatestVersion() !== $this->getVersion()) {
throw new \Exception(sprintf('Cannot download Sass binary. Please verify version `%s` exists for your machine.', $this->getVersion()));
}
throw new \Exception(sprintf('Cannot download Sass binary. Response code: %d', $response->getStatusCode()));
}

$fileHandler = fopen($targetPath, 'w');
foreach ($this->httpClient->stream($response) as $chunk) {
fwrite($fileHandler, $chunk->getContent());
Expand All @@ -86,27 +94,30 @@ public function downloadExecutable(): void

if ($isZip) {
Copy link

Choose a reason for hiding this comment

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

    public function testVersionDownloaded(): void
    {
        $testedVersion = '1.69.5'; // This should differ from the latest version which downloaded by default
        $binary = new SassBinary(binaryDownloadDir: __DIR__.'/fixtures/var/version', binaryVersion: $testedVersion);

        $binary->downloadExecutable();
        exit;
        $this->assertDirectoryExists(__DIR__.'/fixtures/var/version/dart-sass/1.69.5');

        $sassVersionProcess = new Process([__DIR__.'/fixtures/var/version/dart-sass/1.69.5/sass', '--version']);
        $sassVersionProcess->run();
        $this->assertSame(trim($sassVersionProcess->getOutput(), \PHP_EOL), $testedVersion);
    }

the main problem of the failing test testVersionDownloaded (and i assume also the cause for the other failing tests) is this condition. php inspections (ea extended) the awesome plugin of phpstorm warns here that the else block can be extracted as the if block does a return. so for linux we download a tar/tar.gz file and the windows users are "stuck" in the if block and get returned. so there is no renaming to a path with the version inside.

also the zip file has a subdirectory called src this leads to a non-accessible binary

image

i also tried to run the tests on my alpine system, it is quite strange as they also fail. on linux my exit is ignored and all tests are running (i just wanted to create a screenshot of the resulting directory structure after download).

not sure if the code is not that stable or my linux bugging.

i am also confused why this wasn't a problem before,

if (!\extension_loaded('zip')) {
throw new \Exception('Cannot unzip the downloaded sass binary. Please install the "zip" PHP extension.');
throw new \Exception('Cannot unzip the downloaded Sass binary. Please install the "zip" PHP extension.');
}
$archive = new \ZipArchive();
$archive->open($targetPath);
$archive->extractTo($this->binaryDownloadDir);
$archive->extractTo($this->binaryDownloadDir.'/dart-sass');
$archive->close();
unlink($targetPath);

return;
} else {
$archive = new \PharData($targetPath);
$archive->decompress();
$archive->extractTo($this->binaryDownloadDir);
$archive->extractTo($this->binaryDownloadDir.'/dart-sass');

// delete the .tar (the .tar.gz is deleted below)
unlink(substr($targetPath, 0, -3));
}

unlink($targetPath);

$binaryPath = $this->getDefaultBinaryPath();
// Rename the extracted directory to its version
rename($this->binaryDownloadDir.'/dart-sass/dart-sass', $this->binaryDownloadDir.'/dart-sass/'.$this->getVersion());
Copy link

Choose a reason for hiding this comment

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

this is never reached for windows (zip download)


$binaryPath = $this->getDefaultBinaryPath($this->getVersion());
if (!is_file($binaryPath)) {
throw new \Exception(sprintf('Could not find downloaded binary in "%s".', $binaryPath));
}
Expand Down Expand Up @@ -156,11 +167,27 @@ public function getBinaryName(): string

private function buildBinaryFileName(string $os, bool $isWindows = false): string
{
return 'dart-sass-'.self::VERSION.'-'.$os.($isWindows ? '.zip' : '.tar.gz');
return 'dart-sass-'.$this->getVersion().'-'.$os.($isWindows ? '.zip' : '.tar.gz');
}

private function getDefaultBinaryPath(string $version): string
{
return $this->binaryDownloadDir.'/dart-sass/'.$version.'/sass';
}

private function getVersion(): string
{
return $this->cachedVersion ??= $this->binaryVersion ?? $this->getLatestVersion();
}

private function getDefaultBinaryPath(): string
private function getLatestVersion(): string
{
return $this->binaryDownloadDir.'/dart-sass/sass';
try {
$response = $this->httpClient->request('GET', 'https://api.github.com/repos/sass/dart-sass/releases/latest');

return $response->toArray()['tag_name'] ?? throw new \Exception('Cannot get the latest version name from response JSON.');
} catch (\Throwable $e) {
throw new \Exception('Cannot determine latest Dart Sass CLI binary version. Please specify a version in the configuration.', previous: $e);
Copy link

Choose a reason for hiding this comment

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

we have no error handling for rate limits in here, in my tests i get a 403 from github and the resulting error message can be confusing without reading to the caused by part

 Symfonycasts\SassBundle\Tests\SassBuilderTest::testSassOptionQuiet
Exception: Cannot determine latest Dart Sass CLI binary version. Please specify a version in the configuration.

/sass-bundle/src/SassBinary.php:191
/sass-bundle/src/SassBinary.php:181
/sass-bundle/src/SassBinary.php:38
/sass-bundle/src/SassBuilder.php:89
/sass-bundle/tests/SassBuilderTest.php:165

Caused by
Symfony\Component\HttpClient\Exception\ClientException: HTTP/2 403  returned for "https://api.github.com/repos/sass/dart-sass/releases/latest".

}
}
}
3 changes: 2 additions & 1 deletion src/SassBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public function __construct(
private readonly string $cssPath,
private readonly string $projectRootDir,
private readonly ?string $binaryPath,
private readonly ?string $binaryVersion,
Copy link
Member

Choose a reason for hiding this comment

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

hm, this will lead to BC breaks. What about to move this to the end instead?

bool|array $sassOptions = [],
) {
if (\is_bool($sassOptions)) {
Expand Down Expand Up @@ -174,7 +175,7 @@ public function setOutput(SymfonyStyle $output): void

private function createBinary(): SassBinary
{
return new SassBinary($this->projectRootDir.'/var', $this->binaryPath, $this->output);
return new SassBinary($this->projectRootDir.'/var', $this->binaryPath, $this->binaryVersion, $this->output);
}

/**
Expand Down
19 changes: 19 additions & 0 deletions tests/FunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
use Symfony\Component\AssetMapper\AssetMapperInterface;
use Symfony\Component\AssetMapper\MappedAsset;
use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Process\Process;
use Symfonycasts\SassBundle\SassBinary;

class FunctionalTest extends KernelTestCase
{
Expand All @@ -34,6 +36,10 @@ protected function setUp(): void
protected function tearDown(): void
{
unlink(__DIR__.'/fixtures/assets/app.css');
if (file_exists(__DIR__.'/fixtures/var')) {
$filesystem = new Filesystem();
$filesystem->remove(__DIR__.'/fixtures/var');
}
}

public function testBuildCssIfUsed(): void
Expand All @@ -53,4 +59,17 @@ public function testBuildCssIfUsed(): void
$this->assertStringContainsString('color: red', $asset->content);
}
}

public function testVersionDownloaded(): void
{
$testedVersion = '1.69.5'; // This should differ from the latest version which downloaded by default
$binary = new SassBinary(binaryDownloadDir: __DIR__.'/fixtures/var/version', binaryVersion: $testedVersion);

$binary->downloadExecutable();
$this->assertDirectoryExists(__DIR__.'/fixtures/var/version/dart-sass/1.69.5');

$sassVersionProcess = new Process([__DIR__.'/fixtures/var/version/dart-sass/1.69.5/sass', '--version']);
$sassVersionProcess->run();
$this->assertSame(trim($sassVersionProcess->getOutput(), \PHP_EOL), $testedVersion);
}
}
6 changes: 6 additions & 0 deletions tests/SassBuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public function testIntegration(): void
__DIR__.'/fixtures/assets',
__DIR__.'/fixtures',
null,
null
);

$process = $builder->runBuild(false);
Expand All @@ -55,6 +56,7 @@ public function testSassDefaultOptions(): void
__DIR__.'/fixtures/assets',
__DIR__.'/fixtures',
null,
null
);

$process = $builder->runBuild(false);
Expand All @@ -78,6 +80,7 @@ public function testEmbedSources(): void
__DIR__.'/fixtures/assets',
__DIR__.'/fixtures',
null,
null,
[
'embed_sources' => true,
'embed_source_map' => true,
Expand All @@ -104,6 +107,7 @@ public function testSassOptions(): void
__DIR__.'/fixtures/assets',
__DIR__.'/fixtures',
null,
null,
[
'style' => 'compressed',
'source_map' => false,
Expand Down Expand Up @@ -133,6 +137,7 @@ public function testSassBuilderConvertPhpOptions(array $phpOptions, array $expec
__DIR__.'/fixtures/assets',
__DIR__.'/fixtures',
null,
null,
$phpOptions,
);

Expand Down Expand Up @@ -175,6 +180,7 @@ private function createBuilder(array|string $sassFiles, array $options = []): Sa
__DIR__.'/fixtures/assets/dist',
__DIR__.'/fixtures',
null,
null,
$options,
);
}
Expand Down
Loading