inertiajs / inertia-laravel

The Laravel adapter for Inertia.js.
https://inertiajs.com
MIT License
2.03k stars 227 forks source link

Custom `rootView` and shared data set in middleware not taken into account for errors (eg. 404) #176

Open rgehan opened 3 years ago

rgehan commented 3 years ago

The new HandleInertiaRequests middleware exposes a rootView property, allowing to set a custom root view.

It works fine for regular Inertia::render in controllers, but doesn't seem to be taken into account when returning custom error views, as the doc suggests.

In my case, this happens for a 404 error. I imagine Laravel handles such errors differently: the request might not go through the middleware stack.

I just realized that it wouldn't send the shared data to the error page either, nor would it version.

Here's a stack trace:

[2020-10-29 15:26:42] local.ERROR: View [app] not found. {"exception":"[object] (InvalidArgumentException(code: 0): View [app] not found. at /<project>/vendor/laravel/framework/src/Illuminate/View/FileViewFinder.php:137)
[stacktrace]
#0 /<project>/vendor/laravel/framework/src/Illuminate/View/FileViewFinder.php(79): Illuminate\\View\\FileViewFinder->findInPaths('app', Array)
#1 /<project>/vendor/laravel/framework/src/Illuminate/View/Factory.php(138): Illuminate\\View\\FileViewFinder->find('app')
#2 /<project>/vendor/laravel/framework/src/Illuminate/Routing/ResponseFactory.php(85): Illuminate\\View\\Factory->make('app', Array)
#3 /<project>/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(261): Illuminate\\Routing\\ResponseFactory->view('app', Array)
#4 /<project>/vendor/inertiajs/inertia-laravel/src/Response.php(93): Illuminate\\Support\\Facades\\Facade::__callStatic('view', Array)
#5 /<project>/app/Exceptions/Handler.php(51): Inertia\\Response->toResponse(Object(Illuminate\\Http\\Request))
#6 /<project>/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(416): App\\Exceptions\\Handler->render(Object(Illuminate\\Http\\Request), Object(InvalidArgumentException))
#7 /<project>/vendor/laravel/framework/src/Illuminate/Foundation/Http/Kernel.php(114): Illuminate\\Foundation\\Http\\Kernel->renderException(Object(Illuminate\\Http\\Request), Object(InvalidArgumentException))
#8 /<project>/public/index.php(70): Illuminate\\Foundation\\Http\\Kernel->handle(Object(Illuminate\\Http\\Request))
#9 /Users/<user>/.composer/vendor/laravel/valet/server.php(191): require('/Users/<user>/d...')
#10 {main}
"}

// app/Http/Middleware/HandleInertiaRequests.php

class HandleInertiaRequests extends Middleware
{
    protected $rootView = 'layouts.dashboard';

    // ...
}
// app/Exceptions/Handler.php

class Handler extends ExceptionHandler
{
    // ...

    public function render($request, Throwable $e)
    {
        $response = parent::render($request, $e);

        if (in_array($response->status(), [403, 404, 500, 503])) {
            return Inertia::render('public/Error', ['status' => $response->status()])
                ->toResponse($request)
                ->setStatusCode($response->status());
        }

        return $response;
    }
}
claudiodekker commented 3 years ago

Hi,

Thanks for this! You're absolutely right, it doesn't go through the Middleware stack, so the root view never gets changed.

For now, you can work around this by using Inertia::setRootView('customView') directly before the Inertia::render() call, but to fix this I think it might be a good idea to make the Inertia::setRootView method chainable, so that this can all be done in a single expression.. Something like:

Inertia::setRootView('customView')->render('Error', [
    'key' => 'value'
]]);

@reinink what do you think?

rgehan commented 3 years ago

The thing is, that isn't the only thing that's broken.

After working around that issue, I've realized that shared data wasn't set either (neither versioning)

So we'd have to duplicate both setRootView and shared data logic in the error handler too :/

For now, I've just reverted to the old way of setting the root view, and the shared data.

// In a service provider

Inertia::setRootView('layouts.dashboard');

Inertia::share([
  // ...
]);
reinink commented 3 years ago

Hmm, this is an unfortunate side affect to moving to the middleware approach to configuring Inertia. šŸ˜•

As sad as it is to say, this makes me think that we need to go back to the service provider approach. However, I think we can maybe improve that experience by providing an Inertia specific service provider. Something like this:

<?php

namespace App\Providers;

use Inertia\ServiceProvider;
use Illuminate\Http\Request;

class InertiaServiceProvider extends ServiceProvider
{
    /**
     * The root template that's loaded on the first page visit.
     *
     * @see https://inertiajs.com/server-side-setup#root-template
     * @var string
     */
    protected $rootView = 'app';

    /**
     * Determines the current asset version.
     *
     * @see https://inertiajs.com/asset-versioning
     *
     * @return string|null
     */
    public function version(Request $request)
    {
        return parent::version($request);
    }

    /**
     * Defines the props that are shared by default.
     *
     * @see https://inertiajs.com/shared-data
     *
     * @return array
     */
    public function share(Request $request)
    {
        return array_merge(parent::share($request), [
            // your data
        ]);
    }
}
rgehan commented 3 years ago

What was the reasoning with moving from Inertia:: calls in a SP, to a middleware? What problems did it solve?

IMO, the service provider approach was fine, it just lacked a default "skeleton" that users could customize.

With your suggestion, we have the best of both worlds :)

reinink commented 3 years ago

It was argued that it was more proper to do this work in a middleware, since otherwise you're doing Inertia configuration while running console commands.

I personally don't think it matters, as long as this work is all done using lazily evaluated callbacks. And really, this approach is no different than what Taylor does with Fortify (and many other first class Laravel packages):

use Laravel\Fortify\Fortify;

Fortify::loginView(function () {
    return view('auth.login');
});
rgehan commented 3 years ago

It was argued that it was more proper to do this work in a middleware

Naive question, but can't we just use app()->runningInConsole() to shunt the provider when necessary?

I can see where this is coming from though, the argument makes sense. If it had worked for errors, it would have been a perfectly valid solution.

reinink commented 3 years ago

Naive question, but can't we just use app()->runningInConsole() to shunt the provider when necessary?

Probably!

herpaderpaldent commented 3 years ago

I do like @claudiodekker proposal. For that one error page be able to override the root element ... done. I wouldnt know why anyone would like to share props to an error page

claudiodekker commented 3 years ago

Naive question, but can't we just use app()->runningInConsole() to shunt the provider when necessary?

Probably!

Definitely, if we're extending the 'base' service provider (like we're doing with the 'base' middleware now), we can hide all of that logic within the Inertia package šŸ‘

I do like @claudiodekker proposal. For that one error page be able to override the root element ... done. I wouldnt know why anyone would like to share props to an error page

Because error pages might extend layouts, which might use certain shared props (e.g. the user) ;)

reinink commented 3 years ago

I wouldnt know why anyone would like to share props to an error page

Maybe they want to share their app name with the error page? I don't think that just because we can't think of a use-case, that this isn't something we should support.

We also need to consider the version. If you don't set the version, and then you create an Inertia link on your error page, you'll immediately get a full page reload when you click to go elsewhere, which isn't a great experience.

rgehan commented 3 years ago

You've perfectly described my use case:

That definitely needs to be supported, especially since it used to work perfectly fine.

herpaderpaldent commented 3 years ago

Ok, got it. Anywhere, i hope the solution in the end, will be working with packages too. To original implementation did, middleware did so too ...

A way could be: not to support sharing props with middleware way of things. Anyone wanting to such a thing might need to share and set rootview via the service provider

claudiodekker commented 3 years ago

@herpaderpaldent It'd work even better with packages, because you could let your package auto-register a service provider with Laravel, or am I missing something?

rgehan commented 3 years ago

If I'm not mistaken, packages also have the ability to register middlewares (pushMiddleware I think?), so in both cases it should work.

herpaderpaldent commented 3 years ago

Yeyea thats what i mean rightnow middleware and via Inertia::share() works just fine from a package. An inertia serviceprovider from the inertia package makes me thinking

mrmonat commented 3 years ago

The following workaround also works for sharing data but it seems not very nice to "misuse" a middleware this way.

Code in Exception Handler:

Inertia::share((new HandleInertiaRequests)->share($request));
jny986 commented 3 years ago

I have discovered through trouble shooting that if you have the middleware active on a root the middleware overwrites you setting the rootView in controller or in my case the parent controller.

I think that there should be some sort of check to see if there is a root view set and if not set a changeable default.

emargareten commented 3 years ago

šŸ‘€

dmanva commented 3 years ago

I also ran into this problem recently. The only working solution i was able to find add is to add next two lines in custom Exception Handler before calling render:

Inertia::setRootView('my.custom.layout'); Inertia::share((new HandleInertiaRequests)->share($request));

I hope the issue will be solved soon...

reinink commented 3 years ago

Related discussion: https://github.com/inertiajs/inertia/discussions/559

jelleroorda commented 2 years ago

Ran into the same issue, I think I also prefer moving the logic back to a service provider and bailing early in the console.

anburocky3 commented 2 years ago

Welcome to 2022, Like to confirm that, i too face the same problem.

IamSAL commented 2 years ago

Welcome to 2022, Like to confirm that, i too face the same problem.

JenuelDev commented 2 years ago

any update on this?

laserhybiz commented 2 years ago

any updates on any of the issues?

lucasdcrk commented 2 years ago

Same issue here, @mrmonat's solution partially fix the issue as props explicitly set in "HandleInertiaRequests" middleware are passed to the view but not props like user, jetstream, ...

sebastianfeistl commented 10 months ago

I have the same issue and am quite surprised this hasn't been addressed yet.

medabkari commented 9 months ago

Currently facing the same issue, and would like to see this addressed as it has been discussed 3 years now and hasn't been solved yet.

fritz-c commented 6 months ago

Still just a temporary fix, but this helped me add all the Jetstream/user values to the view:

use App\Http\Middleware\HandleInertiaRequests;
use Laravel\Jetstream\Http\Middleware\ShareInertiaData;

(new ShareInertiaData)->handle($request, fn () => null);
Inertia::share((new HandleInertiaRequests)->share($request));
return Inertia::render('public/Error', ['status' => $response->getStatusCode()])
    ->toResponse($request)
    ->setStatusCode($response->getStatusCode());
ahming2000 commented 1 month ago

While researching how to create a custom error page in Inertia.js, I found another solution for this issue.

Previously, I tried to figure out how the Inertia.js package itself could be changed to meet this requirement or resolve this issue. I realized that maybe the Laravel framework should also be adjusted to address this problem. However, there is an expected behavior for the 404 error page, which I discovered from this issue. One of the contributors mentioned:

This is the intended behaviour. You can't expect laravel to guess whether a root mismatch wants sessions, because then 404s on your api would also start sessions.

From this statement, I concluded that we might need to make changes within your own project.

Assuming you are using Laravel 11 with Inertia.js v1.x with Vue 3:

In bootstrap/app.php, you will still need to define the custom exception. You can refer to the Inertia.js official documentation:

use Inertia\Inertia;
use Symfony\Component\HttpFoundation\Response;

...

$exceptions->respond(function (Response $response, Throwable $exception, Request $request) {
    if (!app()->environment(['local', 'testing']) && !in_array($response->getStatusCode(), [500, 503, 403])) {
        $status = $response->getStatusCode();

         // If you have multiple prefix route
        if ($request->is('admin/*')) {
            return Inertia::render('Error/Domain1', compact('status'))
                ->toResponse($request)
                ->setStatusCode($response->getStatusCode());

        } else {
            return Inertia::render('Error/Domain2', compact('status'))
                ->toResponse($request)
                ->setStatusCode($response->getStatusCode());
        }
    } elseif ($response->getStatusCode() === 419) {
        return back()->with([
            'message' => 'The page expired, please try again.',
        ]);
    }

    return $response;
});
...

After that, you need to handle the error manually via the route file, for example at the bottom of your routes/web.php:

/* your other routes */

Route::get('/{any}', function () {
    $status = 404;

    return Inertia::render('Error/Domain1', compact('status'))
        ->toResponse(request())
        ->setStatusCode($status);
})->where('any', '.*');

With this approach, when you navigate to any page that is not found and isn't listed in your routes, you can still retrieve the authenticated user data or session from your custom 404 not found page.

Please let me know if I made any mistakes.