livewire / volt

Volt is an elegantly crafted functional API for Livewire.
https://livewire.laravel.com/docs/volt
MIT License
339 stars 19 forks source link

Use paths from object property, not local variable #95

Closed bookwyrm closed 7 months ago

bookwyrm commented 7 months ago

Volt is already doing the work to save multiple component paths, we just need to use those saved paths when setting up the namespace.

This provides support for easily adding paths to components in custom packages like:

use Illuminate\Support\ServiceProvider;
use Livewire\Volt\Volt;

class PackageServiceProvider extends ServiceProvider
{
    /**
     * Bootstrap the application services.
     */
    public function boot()
    {
        $this->app->booted( function() {
            Volt::mount( __DIR__.'/../resources/views/livewire' );
        } );
    }
}

Fixes #87 and #92 and #94

taylorotwell commented 7 months ago

Drafting pending review from @nunomaduro

inmanturbo commented 7 months ago

This looks far more elegant than #88. But it looks like array_merge() will merge based on the instance, not its path property.

(not added by this pr, but related) https://github.com/livewire/volt/blob/8f4bbbd6f19a4a1e56df4430d4d7eeaf58f546fa/src/MountedDirectories.php#L30

Some things I'd like to mention, that are missing from both this and #88.

If paths can be merged in by packages, then there can be an unknown N paths that could grow quite a bit in an app that depends on some packages that might happen to have some added volt paths.

There should be an easy way to clear or reset the paths mounted by Volt. And in the case of long running apps, or Octane, there should be a check to assert the same path cannot be mounted more than once, or rather that more than one instance of MountedDirectory with the same path will not be merged.

Also, I haven't pulled down myself yet, but with this fix, what happens if a package and the main app both have a component by the same name?

bookwyrm commented 7 months ago

I agree that there are checks that could be added to prevent duplicates and limit growth over time but I wanted to focus on just getting the capability in place first.

Regarding components with the same name, it seems like the behavior would be a bit uncertain. In Livewire\Volt\ ComponentFactory#make() it iterates over the mounted paths to find the matching path and merges in the context for all paths that match.

Then in Illuminate\View\FileViewFinder#findInPaths() Laravel will return the first template that it finds for rendering - so the first mounted path would "win".