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

[2.x] Resolve Closure before checking if a prop implements the Arrayable contract #706

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

rodrigopedra
Copy link
Contributor

On my projects, I use spatie/laravel-view-models to organize complex controller responses.

The Spatie\ViewModels\ViewModel implements the Illuminate\Contracts\Support\Arrayable, so when Inertia can handle a ViewModel instance like a set of props.

When a ViewModel instance is converted to an array, all public properties and methods are converted to array keys. If a public method does not expect any parameters, then it is executed and its value is used. On the other hand, if a public method expects one or more parameters, it is converted to a \Closure.

Currently, the Inertia\Response@resolveArrayableProperties() method, checks if a property is a \Closure after checking if it is an object that implements Illuminate\Contracts\Support\Arrayable, which to me makes little sense as the method is named to "resolve arrayable properties".

Use case

I have the following view model:

<?php

use App\Models\Order;
use App\Models\User;
use App\Support\Browser\Browser;
use App\Support\UserPreferences;
use Spatie\ViewModels\ViewModel;

class SettingsViewModel extends ViewModel
{
    public function __construct(
        // this will be exported directly by ViewModel@toArray()
        public readonly Order $order,

        // this will NOT be exported by ViewModel@toArray()
        private readonly User $user,
    ) {}

    // This will be exported as a string by ViewModel@toArray()
    public function user(): string
    {
        return $this->user->email;
    }

    // **********************************************************
    // This will be exported as a \Closure by ViewModel@toArray()
    public function preferences(Browser $browser): UserPreferences
    {
        return new UserPreferences($browser, $this->user);
    }
}

Where App\Support\UserPreferences also implements Illuminate\Contracts\Support\Arrayable.

Currently, when SettingsViewModel@toArray() is called, SettingsViewModel@preferences is converted to a \Closure, and when Inertia\Response@resolveArrayableProperties() is called, the \Closure is executed after checking if the property implements the Arrayable contract.

I end up with a property holding an empty object, as it doesn't get properly processed.

A simpler example would be returning a \Closure that returns a simple Arrayable instance:

return Inertia::render('IndexPage', [
    'foo' => fn () => new class (['name' => 'Foo']) implements Arrayable {
        public function __construct(
            private readonly array $attrs,
        ) {}

        public function toArray()
        {
            return $this->attrs;
        }
    },
]);

The response currently converts the foo property to an empty object, when I, as a user, would expect its toArray() method to be executed:

{
    "component": "IndexPage",
    "props": {
        "errors": {},
        "foo": {}
    },
    "url": "\/home",
    "version": "4847538ced1a39bde916e313e3dda74a",
    "clearHistory": false,
    "encryptHistory": false
}

This PR:

  • Moves the check if a property is a \Closure before checking if a property implements Arrayable

@rodrigopedra rodrigopedra changed the title Resolve Closure before checking if a prop implements the Arrayable contract [2.x] Resolve Closure before checking if a prop implements the Arrayable contract Jan 15, 2025
@onlime
Copy link

onlime commented Feb 5, 2025

Hi @rodrigopedra thanks for this PR! Did you get any feedback on this? I encounter serious performance issues on a staging environment, and it's blocking me from going live with a larger application that uses Inertia v2 and Spatie's Laravel-Data DTOs (possibly related to this Arrayable resolving) extensively.

SPX Profiler looks like this on a super long running 2100ms request (should be < 100ms in a sane environment):

inertia-706-onlime-spx

As the call stack is split up after the Response::resolveArrayableProperties() call, I guess this is related. Applying your fix though did not help.

Any help or links to related issues greatly appreciated!

@rodrigopedra
Copy link
Contributor Author

Did you get any feedback on this?

No, not yet

@rodrigopedra
Copy link
Contributor Author

Any help or links to related issues greatly appreciated!

You can try cross posting this on Spatie's Laravel-Data DTO repo. Could it be trying to resolve any circular references?

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.

2 participants