inpsyde / assets Goto Github PK

Describe the bug
Lets say you have a multisite. The theme registers its main stylesheet like

new Style('handle', $url, Asset::FRONTEND);

The style will not be loaded on wp-activate.php. The problem is that wp-activate.php defines WP_INSTALLING.

The AssetHookResolver relies on WpContext to define, whether we are in the front end, but for WpContext wp-activate.php is not considered to be in the front end because WP_INSTALLING is true:
https://github.com/inpsyde/wp-context/blob/main/src/WpContext.php#L65-L71

Not sure what the best way to resolve this would be. But at the moment, I think this is the most reasonable solution:

$isFront = $this->context->isFrontoffice() || $this->context->isInstalling()

Describe the bug
While Script::useDependencyExtractionPlugin method returns false Script::resolveDependencyExtractionPlugin method is invokated behind the scene for any version or dependencies retrieving.

To Reproduce
Steps to reproduce the behavior:

1. Setup webpack configuration with DependencyExtractionWebpackPlugin.
2. Build assets.
3. Load assets via PhpFileLoader.
4. Set debug breakpoint to the https://github.com/inpsyde/assets/blob/master/src/Handler/ScriptHandler.php#L70 point.
5. Ensure your script has all dependencies from *.asset.php file.

Expected behavior
Control registering dependencies.

Additional context
Suggestions:

1. Add counterpart Script::disableDependencyExtractionPlugin method.
2. Check Script::$useDependencyExtractionPlugin variable within Script::resolveDependencyExtractionPlugin method.

Is your feature request related to a problem? Please describe.
ES Modules are a growing piece of the ecosystem. https://core.trac.wordpress.org/ticket/12009 still doesn't have a core solution.

Describe the solution you'd like
Add new OutputFilter here.

Describe alternatives you've considered
N/A

Additional context
N/A

Is your feature request related to a problem? Please describe.
I used the AssetManager::asset method to update assets loaded by EncoreEntrypointsLoader but stumbled on the following behavior. When the client doesn't provide second param and assets storage includes several assets shared the same handle (style and script in my case) method silently returns null.

Describe the solution you'd like
Throws LogicException from https://github.com/inpsyde/assets/blob/master/src/AssetManager.php#L171 point with a clear description of what will be wrong. Something like: We found 2 assets with the same handle: %s. Are you forgot to provide $type argument?

Describe alternatives you've considered
Improve documentation.

Hi!

The filters are applied to the inline script tags.

Take the following example:

$testee = new Script('handle', 'foo.js');
$testee->appendInlineScript('var foo = "bar"');
$testee->useDeferFilter();

Will output:

<script defer src='foo.js' id='handle'></script>
<script defer id="handle">
var foo = "bar"
</script>

Could you prevent filters to be applied on inline scripts tags?

Is your feature request related to a problem? Please describe.

I don't really know if this is a feature or a bug.

I'm doing some dynamic rendering in the backend using WordPress AJAX (admin-ajax.php). Problem: my frontend stylesheet isn't enqueued because it's considered a front end request by the hook resolver.

Actually, the isAjax context isn't handled at all. I'm aware admin-ajax.php isn't the best option to deal with this but I have no choice here (plugin).

Is the isAjax something you would agree to handle?

Describe the solution you'd like

Since ajax can used almost anywhere, this context should probably resolve almost every hook.

Describe alternatives you've considered

Extending the hook resolver class but the private $context makes it hard to extend:

• #40

Hi!

Thanks for this awesome library, this is an amazing work.

I'm having trouble removing version of my assets. To be more specific, it seems like you can't remove the query string from the asset URL whatever loader you use (?ver=something).

I'm using a json_manifest to enqueue my assets in production. The whole point of the json_manifest is to cache bust assets without the need to append a query string.

I think this line is to blame : https://github.com/inpsyde/assets/blob/master/src/BaseAsset.php#L113

The only way to remove the version query parameter is to set version to null, which is currently impossible because the version is always casted to string.

Is your feature request related to a problem?

AssetPathResolver may improve with this method.

use Composer\Autoload\ClassLoader;
use ReflectionClass;

    /**
     * @throws \RuntimeException
     */
    public static function getVendorPath(): string
    {
        $reflector = new ReflectionClass(ClassLoader::class);
        $classLoaderPath = $reflector->getFileName();
        if ($classLoaderPath === false) {
            throw new \RuntimeException('Unable to find Composer ClassLoader file.');
        }

        $vendorPath = dirname($classLoaderPath, 2);
        if (!is_dir($vendorPath)) {
            throw new \RuntimeException('Unable to detect vendor path.');
        }

        return $vendorPath;
    }

Describe the desired solution

Use the above method.

Describe the alternatives that you have considered

https://github.com/szepeviktor/package-path/blob/master/src/PackagePath.php#L38

Additional context

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

During REST requests the AssetHookResolver will conclude that we are dealing with a frontend request and return with [ 'wp_enqueue_scripts' ]

This can result in the unwanted execution of script configuration code ( -> in our case, we were seeing some pretty heavy logic used for wp_localize_script being called everywhere, even in REST requests )

However, the solution is not as easy as to add $isRest = defined('REST_REQUEST') to the logic since that constant is only defined after AssetManager bootstrap. I guess you'd need to actually inspect the $REQUEST route to determine a rest request.

As a workaround, we now simply do not configure the script unless we're absolutely sure we need it. But it would be nice if the assets package would remain silent when running "headless"

Is your feature request related to a problem? Please describe.
I use EncoreEntrypointsLoader following https://github.com/inpsyde/assets/blob/master/docs/02%20-%20Loaders.md#webpackloaders. This lets me avoid a lot of bootstrap code. But in some cases, I need to adjust the default assets configuration. For example, add script localization or en queueing condition. For now, it is possible only to retrieve Asset instance by numeric index. The result is some fragile implementation.

{
  "entrypoints": {
    "js/my-cool-js": {
      "js": [
        "../js/my-cool-js.js"
      ]
    }
  }
}

$loader = new EncoreEntrypointsLoader();
/**
* @var array<\Inpsyde\Assets\Script> $assets
*/
$assets = $loader->load($this->entrypoint());
$assets[0]->withLocalize(...);

Describe the solution you'd like
Suggestion is returning from AbstractWebpackLoader::load method handle-indexed array. So we can use next:

$assets['js/my-cool-js']->withLocalize(...);

The handle is a more reliable key than a numeric index that easily changed with new assets.

Describe the bug
With a multi-line inline script attached to an enqueue script handle, I'm hitting this Fatal, traceback https://sentry.io/share/issue/025c3d73635244208dd86f21e3c0006b/

Solved with adding ~...~s modifier to $regex.

To Reproduce
Input $html is:

<root><script src="cxl-ui.bundled.js?ver=1615493663" id="cxl-ui-js" type="module"></script>
<script id="cxl-ui-js-after">
            ((el) => {
                customElements.whenDefined(el).then(() => {
                    el = document.querySelector(el);

                    /**
                     * Populate context menu.
                     *
                     * @see https://vaadin.com/components/vaadin-context-menu/html-examples/context-menu-items-demos
                     */
                    el.contextMenuItems = window.CXL.contextMenuItems;

                    /**
                     * Headroom.
                     *
                     * @see https://github.com/WickyNilliams/headroom.js
                     */
                    window.CXL.headroom = new Headroom(el, {
                        tolerance: {
                          up: 30,
                          down: 30
                        },
                        onPin: () => {
                            const wpAdminBarEl = document.getElementById('wpadminbar');

                            if (wpAdminBarEl) {
                                el.style.top = `\${wpAdminBarEl.offsetHeight}px`;
                            }
                        }
                    });
                    window.CXL.headroom.init();
                });
            })('cxl-marketing-nav');
</script>
</root>

Expected behavior
Regex to succeed.

Additional context
Are we sure we even need these custom root element wraps?

Other than the above, withAttributes() works nicely for type="module":

            $asset_manager->register( $script
                ->enableAutodiscoverVersion()
                ->isInFooter()
                ->withAttributes( [
                    'type' => 'module',
                ] )
            );

-- moved to discussions first -- see: #38

Hi!

When working with other plugins, some suggest to use own hooks to enqueue assets. ACF for example suggests using acf/input/admin_enqueue_scripts. In case of ACF it uses admin_enqueue_scripts with priority 20 to register its assets. After it is done it triggers acf/input/admin_enqueue_scripts.

Is it possible to register assets using other hooks with this library?

Describe the bug
Instead of creating temporary variables with $script = new Script(), could I

$assetManager->register(
  new Script( 'foo', 'bar.js' )
    ->inFooter()
    ->...
);

Wasn't sure if bug or feature, or I am missing something obvious.

I was wondering to understand if there's a particular reason to not use the WordPress Filesystem Api to create and write file/directories and in regards of the permissions why not using the WordPress built in permissions FS_CHMOD_DIR and FS_CHMOD_FILE?

Is your feature request related to a problem? Please describe.
There are assets that need to be only conditionally loaded in specific contexts, such as payment gateway scripts not necessary anywhere outside of WooCommerce is_checkout()

Describe the solution you'd like
New OutputFilter could accept an array of callbacks, or a boolean result to determine whether the asset should be enqueued at all.

This should also work for controlling non-Inpsyde\Asset dependencies. We should be able to initialize any externally enqueued handle to an Inpsyde\Asset object, then apply any OutputFilters as usual.

Describe alternatives you've considered
Not sure, are there any?

Additional context
I'm looking to better control my own and various 3rd party assets enqueued to only specific contexts, to optimized asset payloads for increased page load speed.

Hi!

I'm trying to switch my plugins to oop style. For better testability I want to stay away from WordPress as much as possible. Without knowing of this Composer package, I started to implement my own asset management abstraction (mostly inspired by https://github.com/schlessera/as-speaking). Now I'm trying to use this package.

In my plugin I want to implement a basic meta box:

<?php

declare(strict_types=1);

namespace Plugin\Metabox;

use RegisterableInterface;
use Renderer\RendererInterface;
use Inpsyde\Assets\{Asset, AssetManager, Handler\ScriptHandler, Script};
use Throwable;

final class Github implements RegisterableInterface
{
    private const ID = 'gh-metabox';
    private const JS_HANDLE = 'gh-metabox';

    /** @var RendererInterface */
    private $renderer;

    /** @var AssetManager */
    private $assetManager;

    public function __construct(
        RendererInterface $renderer,
        AssetManager $assetManager
    ) {
        $this->renderer = $renderer;
        $this->assetManager = $assetManager;
    }

    public function register(): void
    {
        $pluginName = plugin_basename(dirname(__DIR__, 2));
        $url = plugins_url() . '/' . $pluginName . '/assets/js/gh.js';

        $script = new Script(self::JS_HANDLE, $url, Asset::BACKEND, ['enqueue' => false]);
        $script->withLocalize('foo', 'bar');
        $this->assetManager->register($script);

        add_action('add_meta_boxes', function () {
            add_meta_box(
                self::ID,
                'Github',
                [$this, 'process']
            );
        });
    }

    public function process(): void
    {
        /** @var Script $script */
        $script = $this->assetManager->asset(self::JS_HANDLE, Script::class);
        if ($script) {
            $script->canEnqueue(true);
            $scriptHandler = $this->assetManager->handlers()[ScriptHandler::class];
            $scriptHandler->enqueue($script);
        }

        try {
            echo $this->renderer->render('gh-metabox');
        } catch (Throwable $e) {
            echo sprintf('<pre>%s</pre>', $e->getMessage());
        }
    }
}

register method is called in add_action('init', ....). The reason I do it this way: I want the JavaScript only loaded when the meta box is rendered.

My question: can assets be enqueued in a simpler way than in the code above?

A configuration array like this will work in version 2.4 but will fail in version 2.5:

[
    'handle' => 'acme-script',
    'type' => Script::class,
    'url' => "assets/script.js",
    'version' => time(),
    'localize' => function (): array {
        return [];
    },
    'translation' => [
        'domain' => 'acme',
        'path' => '/i18n',
    ],
];

The first issue is that the version parameter is now passed directly to BaseAsset::withVersion(string $version); so it has to be a string. Also translation will be passed to Script::withTranslation(string $domain, string $path); but in a inappropriate manner. Finally support for localize has been removed so there should be a documentation that shows how to refactor packages that depends on inpsyde/assets.

I'm working on a PR

Description of the bug

While running static analysis:

1. It would be beneficial to change "an asset implements Asset" to "an asset extend BaseAsset"

   static function (Asset $asset): Asset {
       /** @var BaseAsset $asset */
       return $asset->disableAutodiscoverVersion();
   },

2. Move from array-based configuration to object-oriented configuration

Reproduction instructions

It is hard :)

Expected behavior

see descr

Environment info

No response

Relevant log output

No response

Additional context

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

In recent WP versions, the file plugin.php has not external dependencies.

This was done to ensure that a file that is executed before WordPress is loaded, can requite the file and start adding actions and filters before WordPress loads.

Thanks to that, we could change bootstrap.php to include this fiel in the case add_action is not defined yet.

The current approach can be kept as very last resort fallback, but in theory should never used.

When wp-rename-login is deactivated, and login page is accessed using /wp-login.php, relevant scripts / assets load fine.

Ref:

• assets/src/Util/AssetHookResolver.php

  Line 80 in 51e564b

  case $this->context->isLogin():

• https://github.com/inpsyde/wp-context/blob/1e30337f9c43e0d4c3b882a6ff76fff108ea2520/src/WpContext.php#L139