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

Use paths from object property, not local variable #95

Merged

Conversation

bookwyrm
Copy link
Contributor

@bookwyrm bookwyrm commented Mar 4, 2024

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

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:

```php
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' );
        } );
    }
}
```
@taylorotwell
Copy link
Collaborator

Drafting pending review from @nunomaduro

@inmanturbo
Copy link

inmanturbo commented Mar 5, 2024

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)

$this->paths = array_merge($this->paths, $paths->all());

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
Copy link
Contributor Author

bookwyrm commented Mar 5, 2024

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".

@bookwyrm bookwyrm marked this pull request as ready for review March 5, 2024 21:38
@taylorotwell taylorotwell merged commit e5316b4 into livewire:main Mar 6, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling Volt::mount a second time negates initial call
4 participants