Skip to content

Comments

[8.x] Autowiring Blade Class Component Properties#34471

Closed
imliam wants to merge 1 commit intolaravel:masterfrom
imliam:autowire-blade-components
Closed

[8.x] Autowiring Blade Class Component Properties#34471
imliam wants to merge 1 commit intolaravel:masterfrom
imliam:autowire-blade-components

Conversation

@imliam
Copy link
Contributor

@imliam imliam commented Sep 23, 2020

Edit: Oops, just noticed that I branched from master instead of 8.x - will fix the conflicts and point the base to 8.x once that's sorted out. Pointed the base to master for now so it's easy to see the diff.

What is this?

This pull request introduces "autowiring" of properties on Blade component classes when a class component uses the new AutowiresProperties trait.

This allows properties on the component class to be automatically set from the provided data, instead of needing to go through the constructor just to be set.

// Class component
class Input extends Component
{
    use AutowiresProperties;

    public $type = 'text';

    public function render()
    {
        <<<'BLADE'
            <input type="{{ $type }}" {{ $attributes }}>
        BLADE;
    }
}

// Component Usage
<x-input type="email" class="form-input" />

Following the same convention as Livewire v2, it also now lets you define a mount() on the component class. If present, it will be called AFTER the autowiring step (while __construct() is called BEFORE autowiring the properties), accepting parameters of any attribute that was passed through, or injecting dependencies from the container.

This lets us do extra logic if we need to after the autowiring step.

// Class component
class Input extends Component
{
    // ...

    public function mount($type = 'text')
    {
        if (!in_array($type, ['text', 'email', 'number', 'password'])) {
            $this->type = 'text';
        }
    }
}

// Component Usage
<x-input type="invalid-type" />

Why do we need this?

Personally, when writing certain Blade component classes, I have ended up having to repeat properties 3 times over; once to define the property, once to accept it in the constructor, and once to set the property value in the constructor.

That's okay for one or two parameters/properties, but on larger components it becomes unbearable. This is something that's bugged me quite a lot and would love to be able to move away from to clean up some unneeded code.

Livewire recently addressed this in the v2 release through the same approach; automatically wiring any properties with the named values that would normally just be manually set.

How could this behaviour be improved in future versions?

Currently, this PR forces users to use the AutowiresProperties trait on their component to enable this functionality, just to make it available. This is because I can't find a good way to make it implicit on the base component class without being a breaking change in some way; notably that properties already set in an existing constructor could be overridden by the autowiring logic, and I don't believe there's a way to check if a class property is "dirty" through reflection to prevent that.

If we wanted to make this behaviour implicit in a future major version like 9.x, the trait method could be moved directly to the base component class instead, and tell people to always use the mount() method instead of __construct() to handle property values they want to manually set.

Thoughts?

Keen to hear feedback on the PR or if other people want this kind of functionality too.

There's a couple of failing tests at the time of writing but I'll sort them out.

@imliam imliam changed the base branch from 8.x to master September 23, 2020 00:27
@roni-estein
Copy link

Have you tried accomplishing this via anonymous components and and the attribute functions?

1 similar comment
@roni-estein
Copy link

Have you tried accomplishing this via anonymous components and and the attribute functions?

@Larsklopstra
Copy link
Contributor

I need this

@GrahamCampbell GrahamCampbell marked this pull request as draft September 23, 2020 08:08
@GrahamCampbell GrahamCampbell changed the title [8.x] Autowiring Blade Class Component Properties (WIP) [8.x] Autowiring Blade Class Component Properties Sep 23, 2020
@brendt
Copy link
Contributor

brendt commented Sep 23, 2020

A thought about the main problem you mention:

I have ended up having to repeat properties 3 times over; once to define the property, once to accept it in the constructor, and once to set the property value in the constructor.

PHP 8 will solve this with constructor property promotion. Thinking about it some more, that's probably the route I'll prefer compared to added magic:

class Input extends Component
{
    public function __construct(
        public string $type = 'text',
    ) {}
    

    public function render()
    {
        <<<'BLADE'
            <input type="{{ $type }}" {{ $attributes }}>
        BLADE;
    }
}

@ryangjchandler
Copy link
Contributor

A thought about the main problem you mention:

I have ended up having to repeat properties 3 times over; once to define the property, once to accept it in the constructor, and once to set the property value in the constructor.

PHP 8 will solve this with constructor property promotion. Thinking about it some more, that's probably the route I'll prefer compared to added magic:

class Input extends Component
{
    public function __construct(
        public string $type = 'text',
    ) {}
    

    public function render()
    {
        <<<'BLADE'
            <input type="{{ $type }}" {{ $attributes }}>
        BLADE;
    }
}

That's true, but how many people are going to be upgrading to PHP 8 straight away? Realistically, I don't think many people will be.

}

if (method_exists($this, 'mount')) {
app()->call([$this, 'mount'], $allAttributes);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend avoiding the app() helper here too - Container::getInstance() makes more sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both of those are kinda hacky, actually. Assumes there is only one container instance and it's shared with the entire app.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While that sort of thing is fine in your own app code, and in the foundation namespace, it should be avoided in core components.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, from looking through the code base, the Container::getInstance() approach is used in a lot of places.

@brendt
Copy link
Contributor

brendt commented Sep 23, 2020

That's true, but how many people are going to be upgrading to PHP 8 straight away? Realistically, I don't think many people will be.

@ryangjchandler the question is: is this feature worth the added maintenance cost when it's replaceable in the foreseeable future.

By the way, the answer to that question might very well be "yes". I just wanted to mention the upcoming alternative.

@taylorotwell
Copy link
Member

I think I'll hold off on this for now. Couple reasons. First, it will be easier in PHP 8 as noted above. Secondly, it adds several more Reflection and collection operations to every new auto-wiring component on the page. If we have a table of 500 rows on a page and each row uses 5 components, now we are doing these operations 2,500 times to render the page - so I would like to keep the component layer as light as possible.

We will be able to get rid of the extra repetition in PHP 8 with no performance penalty.

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.

7 participants