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

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

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ericp-mrel
Copy link
Contributor

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

@LukeTowers
Copy link
Member

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

@bennothommo bennothommo marked this pull request as ready for review August 10, 2024 13:32
@bennothommo bennothommo marked this pull request as draft August 10, 2024 13:32
@ericp-mrel
Copy link
Contributor Author

@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
Copy link
Member

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

@LukeTowers LukeTowers added enhancement PRs that implement a new feature or substantial change Status: In Progress labels Aug 18, 2024
@ericp-mrel
Copy link
Contributor Author

@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
Copy link
Member

@bennothommo or @jaxwilko might have some input here

@jaxwilko
Copy link
Member

jaxwilko commented Aug 19, 2024

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:
image

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 :)

Also fixes code formatting errors.
{
static $manifests = [];

$theme = Theme::getActiveTheme();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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]));
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

@ericp-mrel any thoughts on the above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@jaxwilko do you have any input on the above?

Copy link
Member

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

@LukeTowers
Copy link
Member

@ericp-mrel what's left on this?

@ericp-mrel
Copy link
Contributor Author

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

Screenshot 2024-11-13 at 5 18 49 PM

@LukeTowers
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that implement a new feature or substantial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants