feat: Add an optional disableDependencyExtraction() method

[nlemoine]

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes/features)
• Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Adds ability to disable automatic dependency extraction file lookups

What is the current behavior? (You can also link to an open issue here)

Dependency extraction always performs I/O operations (DirectoryIterator scans, file reads) to search for .asset.json/.asset.php files, even when not needed. Which can lead to (minor) performance issues.

What is the new behavior (if this is a feature change)?

Users can disable dependency extraction lookups for better performance when manually managing dependencies:

$script->disableDependencyExtraction();

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No. Dependency extraction remains enabled by default. This is an opt-in performance optimization.

I'll update the docs if you're ok with that change.

[widoz]

I'm not sure about the strategy, yet another configurable field. If we could have the $dependencies by default as null we could have an automated chained strategy to rely on the given properties, version and dependencies and falling back to the Dependencies Extraction. But the field $dependencies is protected and by default an empty array.

I'm not that confided we can approach a change in the default type for that field, though, looking at the process I'm worrying about the different ways the Assets instances can be passed here and there and the fact that, the resolution strategy can change at runtime right after the asset as been configured and already shared or, I can query for a field e.g. version before I can configure further the asset and at that point the fields are already filled and the additional option would be useless.

I'm not saying I don't like the concept but I don't trust the execution, to me disabling the Dependencies Extraction should be something happening at the beginning of the instantiation of an Asset object and should never change.

[nlemoine]

Thanks for the feedback @widoz

If we could have the $dependencies by default as null we could have an automated chained strategy to rely on the given properties, version and dependencies and falling back to the Dependencies Extraction. But the field $dependencies is protected and by default an empty array.

Having such implicit behavior (null = disable, [] = enable) would be even worse IMHO.

I'm not that confided we can approach a change in the default type for that field, though, looking at the process I'm worrying about the different ways the Assets instances can be passed here and there and the fact that, the resolution strategy can change at runtime right after the asset as been configured and already shared or, I can query for a field e.g. version before I can configure further the asset and at that point the fields are already filled and the additional option would be useless.

I share the concern about runtime mutation, but the same applies to other Asset props (version, filePath, etc.) because by design, all props are mutable.

You currently have the exact same flaw with autodiscoverVersion():

$asset->version(); // Triggers https://github.com/inpsyde/assets/blob/9bf2a427270a3c803544f807f4b32fe10d8c834b/src/BaseAsset.php#L140-L145
// Later on/elsewhere
$asset->disableAutodiscoverVersion();
echo $asset->version();
// 1764620994 (filemtime)

I'm not sure about the strategy, yet another configurable field.

I understand your concerns but I tried to stick to the current library design and not introduce breaking changes. I tend to pick consistency over improvement when the latter implies large code refactoring.

Would a __construct arg on Script seem a better approach to you? Something like bool $disableDependencyExtraction = false as the last parameter — immutable once set, no runtime surprises.

[widoz]

Hi @nlemoine, forgive me for the late reply.

Having such implicit behavior (null = disable, [] = enable) would be even worse IMHO.

Well, no the internal logic would change the value to null whenever there's an empty dependency list.

You currently have the exact same flaw with autodiscoverVersion():

That's true, but a wrong script version would not break the script from being enqueued, not having deps is a different problem. This leads me to your other comment

Would a __construct arg on Script seem a better approach to you? Something like bool $disableDependencyExtraction = false as the last parameter — immutable once set, no runtime surprises.

Yes, I would prefer that, and we could do $this->dependencyExtractionEnabled and $this->resolveDependencyExtractionPlugin();

cc @Chrico

[nlemoine]

Ciao @widoz,

I moved the flag to Script constructor and switched from disable to enable logic. Let me know if this fits your requirements.

After a second thought on this, I think the dependency extraction should probably check for files without globing. Since filenames are predictable, a loop of file checks would probably be more efficient. But that will be for a later PR.

[widoz]

Hi @nlemoine changes lgtm, can you also add them to the [ScriptModule](https://github.com/inpsyde/assets/blob/master/src/ScriptModule.php) class?

[nlemoine]

@widoz Sorry, i forgot about that class, this is done!

I would appreciate if you could also take a look at #69 which is a bug and more critical to my use case 🙏 Thanks!

[widoz]

LGTM thank you @nlemoine

@Chrico if you want to give it a quick look.

[widoz]

Merged, thank you @nlemoine

[nlemoine]

Grazie @widoz !