Adds a `mix` Twig filter for Laravel Mix theme assets. #1179

ericp-mrel · 2024-08-04T02:31:32Z

This PR adds support for a Laravel mix Twig mix filter.

LukeTowers · 2024-08-04T05:32:45Z

@ericp-mrel are you able to add tests for this and a docs PR?

ericp-mrel · 2024-08-17T21:05:46Z

@LukeTowers I'm trying to create some tests, but I'm running into an issue where I cannot override the base path that's used by the $theme->getPath() function. I've tried overriding all of the config settings in the setup() function, but this doesn't seem to have any effect on anything that relies on the themes_path() helper function.

Do you have any tips on how I can get this to work?

<?php

namespace System\Tests\Classes\Asset;

use Cms\Classes\Theme;
use System\Classes\Asset\Mix;
use System\Classes\Asset\PackageManager;
use System\Tests\Bootstrap\TestCase;
use Winter\Storm\Support\Facades\Config;
use Winter\Storm\Support\Facades\Event;
use Winter\Storm\Support\Facades\File;

class MixTest extends TestCase
{
    protected string $themePath;

    protected string $originalThemesPath = '';

    protected function setUp(): void
    {
        parent::setUp();

        if (!is_dir(base_path('node_modules'))) {
            $this->markTestSkipped('This test requires node_modules to be installed');
        }

        if (!is_file(base_path('node_modules/.bin/mix'))) {
            $this->markTestSkipped('This test requires the mix package to be installed');
        }

        $this->originalThemesPath = Config::get('cms.themesPath');
        Config::set('cms.themesPath', '/modules/system/tests/fixtures/themes');

        $this->themePath = base_path('modules/system/tests/fixtures/themes/mixtest');

        Config::set('cms.activeTheme', 'mixtest');

        Event::flush('cms.theme.getActiveTheme');
        Theme::resetCache();

        PackageManager::instance()->registerPackage(
            'theme-mixtest',
            $this->themePath . '/winter.mix.js'
        );
    }

    protected function tearDown(): void
    {
        File::deleteDirectory('modules/system/tests/fixtures/themes/mixtest/assets/dist');
        File::delete('modules/system/tests/fixtures/themes/mixtest/mix-manifest.json');

        Config::set('cms.themesPath', $this->originalThemesPath);

        parent::tearDown();
    }

    public function testGeneratesVersionedUrl(): void
    {
        $this->artisan('mix:compile', [
            'theme-mixtest',
            '--manifest' => 'modules/system/tests/fixtures/npm/package-mixtest.json',
            '--disable-tty' => true,
        ])->assertExitCode(0);

        $theme = Theme::getActiveTheme();

        dd($theme->getPath()); // This keeps pointing to /modules/cms instead of /modules/system
    }

    public function testThemeCanOverrideMixManifestPath(): void
    {

    }
}

LukeTowers · 2024-08-18T05:11:26Z

@ericp-mrel this might be useful to you: d27dbbc. There's two separate config keys, cms.themesPath and cms.themesPathLocal.

ericp-mrel · 2024-08-18T23:07:34Z

@LukeTowers That didn't seem to make any difference, after poking around more in the container classes I found out there was a function called setThemesPath() that was being run as part of the bootstrap process. So, I had to add the following line to get the themes path to resolve correctly for the tests.

$this->app->setThemesPath(Config::get('cms.themesPathLocal'));

I'm not sure if there's a better way to handle setting that value in the Container?

Full File:

<?php

namespace System\Tests\Classes\Asset;

use Cms\Classes\Theme;
use Illuminate\Support\Facades\App;
use System\Classes\Asset\Mix;
use System\Classes\Asset\PackageManager;
use System\Tests\Bootstrap\TestCase;
use Winter\Storm\Support\Facades\Config;
use Winter\Storm\Support\Facades\Event;
use Winter\Storm\Support\Facades\File;

class MixTest extends TestCase
{
    protected string $themePath;

    protected string $originalThemesPath = '';

    protected string $originalThemesPathLocal = '';

    protected function setUp(): void
    {
        parent::setUp();

        if (!is_dir(base_path('node_modules'))) {
            $this->markTestSkipped('This test requires node_modules to be installed');
        }

        if (!is_file(base_path('node_modules/.bin/mix'))) {
            $this->markTestSkipped('This test requires the mix package to be installed');
        }

        $this->originalThemesPath = Config::get('cms.themesPath');
        Config::set('cms.themesPath', '/modules/system/tests/fixtures/themes');

        $this->originalThemesPathLocal = Config::get('cms.themesPathLocal');
        Config::set('cms.themesPathLocal', base_path('modules/system/tests/fixtures/themes'));
        $this->app->setThemesPath(Config::get('cms.themesPathLocal'));

        $this->themePath = base_path('modules/system/tests/fixtures/themes/mixtest');

        Config::set('cms.activeTheme', 'mixtest');

        Event::flush('cms.theme.getActiveTheme');
        Theme::resetCache();
    }

    protected function tearDown(): void
    {
        File::deleteDirectory('modules/system/tests/fixtures/themes/mixtest/assets/dist');
        File::delete('modules/system/tests/fixtures/themes/mixtest/mix-manifest.json');

        Config::set('cms.themesPath', $this->originalThemesPath);

        parent::tearDown();
    }

    public function testGeneratesVersionedUrl(): void
    {
        $this->artisan('mix:compile', [
            'theme-mixtest',
            '--manifest' => 'modules/system/tests/fixtures/npm/package-mixtest.json',
            '--disable-tty' => true,
        ])->assertExitCode(0);

        $theme = Theme::getActiveTheme();

        dd($this->app->make('path.themes'), $theme->getPath(), $theme->assetUrl('assets/dist/css/theme.css'), (string) Mix::mix('assets/dist/css/theme.css'));
    }

    public function testThemeCanOverrideMixManifestPath(): void
    {

    }
}

LukeTowers · 2024-08-19T07:23:19Z

@bennothommo or @jaxwilko might have some input here

jaxwilko · 2024-08-19T12:37:20Z

Hi @ericp-mrel, I've just set up a dummy test which might help you.

I created a theme fixture as modules/system/tests/fixtures/themes/mixtest:

The mix file for this is:

const mix = require('laravel-mix');

mix.setPublicPath(__dirname);

mix
    .css('assets/css/theme.css', 'assets/dist/theme.css')
    .js('assets/javascript/theme.js', 'assets/dist/theme.js')
    .options({
        manifest: true,
    });

Then a custom manifest specifically for this test as modules/system/tests/fixtures/npm/package-mixtheme.json:

{
    "workspaces": {
        "packages": [
            "modules/system/tests/fixtures/themes/mixtest"
        ]
    },
    "devDependencies": {
        "laravel-mix": "^6.0.41"
    }
}

Then I added the following to MixCompileTest::__constructor():

$this->themePath = base_path('modules/system/tests/fixtures/themes/mixtest');

// Add our testing theme because it won't be auto discovered
\System\Classes\Asset\PackageManager::instance()->registerPackage(
    'theme-mixtest',
    $this->themePath . '/winter.mix.js',
);

Then finally the test code:

public function testVersioning(): void
{
    $this->artisan($this->command, [
        '--manifest' => 'modules/system/tests/fixtures/npm/package-mixtheme.json',
        '--package' => 'theme-mixtest',
        '--disable-tty' => true
    ])
        ->assertExitCode(0);

    $manifestPath = $this->themePath . '/mix-manifest.json';

    // Validate the file exists
    $this->assertFileExists($manifestPath);

    // Check the contents of the file
    $contents = File::get($manifestPath);
    $this->assertStringContainsString('/assets/dist/theme.css', $contents);

    // Clean up
    File::deleteDirectory($this->themePath . '/assets/dist');
    File::delete($manifestPath);
}

If you have any questions feel free to @ me :)

…tUrl()` function.

Also fixes code formatting errors.

LukeTowers · 2024-08-22T17:57:02Z

modules/system/classes/asset/Mix.php

+    {
+        static $manifests = [];
+
+        $theme = Theme::getActiveTheme();


This should probably be in the CMS module instead since it requires the CMS module.

Do you think it would be better if I were to update the Mix class, so it acts similar to the Vite class? Where you would pass the path of the asset and then the package name of where it belongs to, so it doesn't have to rely on the Theme class and then can work for plugins as well?

It would then be used like this:

<script src="{{ mix('assets/dist/js/theme-mytheme.js', 'theme-mytheme') }}" defer></script>

Mix class rewrite:

<?php namespace System\Classes\Asset; use Exception; use Illuminate\Support\HtmlString; use InvalidArgumentException; use Winter\Storm\Exception\SystemException; use Winter\Storm\Support\Facades\Url; use Winter\Storm\Support\Str; class Mix { protected static array $manifests = []; public function __invoke(string $path, string $package): HtmlString|string { if (!$package) { throw new InvalidArgumentException('A package must be passed'); } // Normalise the package name $package = strtolower($package); if (!($compilableAssetPackage = PackageManager::instance()->getPackages('mix')[$package] ?? null)) { throw new SystemException('Unable to resolve package: ' . $package); } $manifestPath = public_path($compilableAssetPackage['path'] . '/mix-manifest.json'); if (!isset(static::$manifests[$manifestPath])) { if (!is_file($manifestPath)) { throw new Exception("The Mix manifest does not exist."); } static::$manifests[$manifestPath] = json_decode(file_get_contents($manifestPath), true); } $manifest = static::$manifests[$manifestPath]; $path = Str::start($path, '/'); if (!isset($manifest[$path])) { $exception = new Exception("Unable to locate Mix file: $path"); if (!app('config')->get('app.debug')) { report($exception); return $path; } else { throw $exception; } } return new HtmlString(Url::asset($compilableAssetPackage['path'] . $manifest[$path])); } }

@ericp-mrel any thoughts on the above?

@LukeTowers Did you see my comment / code about re-writing this to work similar to the Vite class?

@jaxwilko do you have any input on the above?

@ericp-mrel looked at your suggestion and I like it, I would leave the mix filter being registered by the CMS module however so that the default value of the package argument could be the current theme. Then if anyone wants to use it in plugins they can just use the Mix class directly (or if you see value in it add a addMix() method like we have addVite() in the AssetMaker trait).

This is probably the approach I'd go with (in terms of replicating the vite() method functionality), however I'd probably try and keep the signatures the same (add support for array input with different file types) and return the entire tag i.e.

Twig:

{{ mix(['path/to/example.js', 'path/to/example.css'], 'theme-example') }}

Rendered output:

<link rel="modulepreload" href="https://example.com/path/to/example-hash.js"> <script type="module" src="https://example.com/path/to/example-hash.js"></script> <link rel="preload" as="style" href="https://example.com/path/to/example-hash.css"> <link rel="stylesheet" href="https://example.com/path/to/example-hash.css">

This will allow people to swap between vite and mix easier in the future and makes the documentation simplier as they will both function in the same way.

It's probably worth adding support for passing the manifest file as an extra param on the tag, as the file doesn't necessarily always have the same name (see: #1167) i.e.

{{ mix(['path/to/example.js', 'path/to/example.css'], 'theme-example') }} // vs {{ mix(['path/to/example.js', 'path/to/example.css'], 'theme-example', 'my-custom-manifest.json') }} // or {{ mix(['path/to/example.js', 'path/to/example.css'], 'theme-example', 'path/to/my-custom-manifest.json') }}

We can assume the default, but if a custom manifest name/path is passed it will allow for user overrides.

modules/system/classes/asset/Mix.php

LukeTowers · 2024-11-13T22:13:24Z

@ericp-mrel what's left on this?

ericp-mrel · 2024-11-13T23:19:50Z

@LukeTowers Just waiting on a response to my question from here:

LukeTowers · 2024-11-14T04:09:31Z

@ericp-mrel I can't see that response until you submit your review, that's what the "Pending" status flag next to it means 😄

Adds a mix Twig filter for Laravel Mix theme assets.

d00b9f6

bennothommo marked this pull request as ready for review August 10, 2024 13:32

bennothommo marked this pull request as draft August 10, 2024 13:32

LukeTowers added enhancement PRs that implement a new feature or substantial change Status: In Progress labels Aug 18, 2024

Started adding working tests for Mix helper class.

4f89e79

ericp-mrel added 4 commits August 21, 2024 20:15

Generate asset URLs directly rather than relying on the Theme's `asse…

24caf18

…tUrl()` function.

Finish creating Mix class tests.

5692e71

Also fixes code formatting errors.

Add test for Laravel Mix Twig filter.

ead0189

Add additional tests for Mix class.

5fb257f

LukeTowers reviewed Aug 22, 2024

View reviewed changes

modules/system/classes/asset/Mix.php Outdated Show resolved Hide resolved

LukeTowers reviewed Aug 22, 2024

View reviewed changes

modules/system/classes/asset/Mix.php Outdated Show resolved Hide resolved

Fix code styling.

3100bbc

bennothommo removed the Status: In Progress label Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a `mix` Twig filter for Laravel Mix theme assets. #1179

Adds a `mix` Twig filter for Laravel Mix theme assets. #1179

ericp-mrel commented Aug 4, 2024

LukeTowers commented Aug 4, 2024

ericp-mrel commented Aug 17, 2024

LukeTowers commented Aug 18, 2024

ericp-mrel commented Aug 18, 2024

LukeTowers commented Aug 19, 2024

jaxwilko commented Aug 19, 2024 •

edited

Loading

LukeTowers Aug 22, 2024

ericp-mrel Sep 13, 2024

LukeTowers Sep 26, 2024

ericp-mrel Sep 26, 2024

LukeTowers Nov 14, 2024

LukeTowers Nov 20, 2024

jaxwilko Nov 24, 2024

LukeTowers commented Nov 13, 2024

ericp-mrel commented Nov 13, 2024

LukeTowers commented Nov 14, 2024

Adds a mix Twig filter for Laravel Mix theme assets. #1179

Are you sure you want to change the base?

Adds a mix Twig filter for Laravel Mix theme assets. #1179

Conversation

ericp-mrel commented Aug 4, 2024

LukeTowers commented Aug 4, 2024

ericp-mrel commented Aug 17, 2024

LukeTowers commented Aug 18, 2024

ericp-mrel commented Aug 18, 2024

LukeTowers commented Aug 19, 2024

jaxwilko commented Aug 19, 2024 • edited Loading

LukeTowers Aug 22, 2024

Choose a reason for hiding this comment

ericp-mrel Sep 13, 2024

Choose a reason for hiding this comment

LukeTowers Sep 26, 2024

Choose a reason for hiding this comment

ericp-mrel Sep 26, 2024

Choose a reason for hiding this comment

LukeTowers Nov 14, 2024

Choose a reason for hiding this comment

LukeTowers Nov 20, 2024

Choose a reason for hiding this comment

jaxwilko Nov 24, 2024

Choose a reason for hiding this comment

LukeTowers commented Nov 13, 2024

ericp-mrel commented Nov 13, 2024

LukeTowers commented Nov 14, 2024

Adds a `mix` Twig filter for Laravel Mix theme assets. #1179

Adds a `mix` Twig filter for Laravel Mix theme assets. #1179

jaxwilko commented Aug 19, 2024 •

edited

Loading