[2.x] Inertia: Automatically publish & register Inertia Middleware

[claudiodekker]

Solves #326

This PR automatically publishes and registers the new HandleInertiaRequests Inertia Middleware with the Http Kernel, to enable automatic asset versioning (cache busting) and error sharing as of default. To take full advantage of this, it also enables versioning as of default in the webpack.mix.js.

Additionally, because Jetstream automatically registers the ShareInertiaData middleware on the 'web' group as priority middleware, this PR also adds logic to ensure the HandleInertiaRequests middleware always runs directly after it when it exists. This gives the user the ability to overwrite the Inertia built-in defaults (such as the user key) if they want to.

Finally, because this Middleware will exist entirely in userland, it allows the user to change it in to any configuration they want, including disabling it entirely.

[claudiodekker]

Alright, so, tons of changes. Let me guide you through it.

54db22a

First, I've solved the issue of the Inertia::version being part of the vendor, by moving it to a newly-published InertiaServiceProvider.

Because as a result of this we ended up with a lot of duplicate methods (installFortifyServiceProvider, installJetstreamServiceProvider, installInertiaServiceProvider), I've refactored this methods to a single installServiceProvider($name, $after) method.

7025200

Next, I ran into the dilemma that while Jetstream's Inertia::share would be nice in userland, doing so would cause a lot of repetition. We already had two different files (JetstreamServiceProvider.php, JetstreamWithTeamsServiceProvider.php), and making this change would result in 4 files, as we'd also need one for Inertia as well as one for Inertia with Teams.

To solve this issue, I remembered a thing I learned from @taylorotwell (Laravel ☁️ ) where he used Blade views to 'generate' scripts. I basically did the same thing here by moving the JetstreamServiceProvider into a single 'template', and rendering the file into the state in which it's needed on-the-fly.

Hope this makes sense!

[taylorotwell]

To me it feels odd to call Inertia::share outside of a web request context. For example, I would pretty much expect it to be done in a middleware or controller where there is definitely an HTTP request happening. A console command will fire up all service providers and this will call Inertia::share in that case.

[claudiodekker]

Yeah, I definitely understand, but the inability to define your own defaults and override the user there is kind of problematic, as it'd force you to pretty much drop Jetstream in that situation.

Personally, I don't think its too big of a problem to have them bootstrap like this. Yes, it's maybe not the most efficient, but I kind of see Inertia::share similar to registering container bindings that aren't resolved yet

Perhaps there's something else we can try? Maybe even a change that we can make upstream in Inertia's Laravel adapter?

I'll see if I can find a better solution. In either case, thanks for the feedback 👍

Alright, I went through the Laravel request pipeline, and having it as part of the priority middleware is indeed by far the most preferable approach. I'll revert some of these changes.

[claudiodekker]

Done. I've reverted some of the changes and will update the original message of this PR to reflect this.

In addition, I've briefly had a chat about this topic with @reinink, since we're using Inertia::share inside the AppServiceProvider on our Inertia best-practices / demo repository (PingCRM). I'll create a PR to that repository over the next few days to use Middleware instead, similar to what you're doing here. 👍

[taylorotwell]

Is this ready? If so, can you mark it as ready. I have my GitHub set to filter out draft PRs.

[claudiodekker]

Yeah, it's ready, but @reinink asked me to re-draft it so he could talk to you about it..

[taylorotwell]

Status? Should this be made active?

[claudiodekker]

Sorry, forgot to post an update here.

Because of the conversation that happened here, we've decided to change Inertia's Laravel adapter to use Middleware as well, resulting in this PR: inertiajs/inertia-laravel#161

The plan is to release that PR tomorrow, at which point I'll update this PR and will mark it ready for review.

Basically, the idea is that this PR will call artisan inertia:middleware (which is similar to artisan make:middleware) to 'generate' an Inertia middleware with certain defaults (automatic versioning like we were introducing here + sharing of errors), which will allow us to remove the entire stub from this PR, making it a lot easier to maintain within Jetstream.

[claudiodekker]

@taylorotwell Alright, everything seems to be working 👍

I also went ahead and split the webpack.mix.js file to be stack-specific, because we already had an ongoing discussion about this over here anyway, and I also didn't really like the idea of touching something that Livewire is also affected by.

[taylorotwell]

@claudiodekker Can this be re-targeted to master so we can release all of these Inertia improvements at once?

[claudiodekker]

@taylorotwell Sure, give me 5 mins.. verifying that everything will keep working

[claudiodekker]

@taylorotwell Done 👍

[taylorotwell]

I have a feeling all that middleware replacement using PHP_EOL isn't going to work on Windows. 👀

[taylorotwell]

Particularly here. On Windows I'm not sure this Str::after call will return what you want.

[claudiodekker]

Yeah, I think you're right.. I totally forgot about Windows doing things like this differently.

Let me see if I can adjust this.

[taylorotwell]

Honestly my gut would be to just replace the HTTP kernel entirely with a stub that contains exactly what you want. It will have some duplication with Laravel itself but would prevent the need for this complicated replace logic.

[claudiodekker]

@taylorotwell What about now? Do you still prefer creating a HTTP kernel stub?

[taylorotwell]

I get this. I assume because you're installing the Inertia dependency but then calling a command in the same process such that for that process it is not installed yet. Make sense?

[taylorotwell]

Note that error would only occur on a very fresh install so you might not be seeing it if your testing on app that already had Inertia installed.

[claudiodekker]

Ah shoot! Yeah, I get what you're saying, it's re-using the container that's already booted.

[taylorotwell]

Probably you would need to use Symfony Process to run the command.

[taylorotwell]

OK seems fine to me now. Good to merge?

[claudiodekker]

Yep, sure. I just got a pretty crazy idea that might actually be a better alternative to the 'patching' that I'm doing now, but I can just PR that later as it's not breaking 👍

[claudiodekker]

@taylorotwell In case you were curious: I played around with that idea I mentioned for a bit, but I don't think the end result would be better.

Basically, what I wanted to do, is use ReflectionClass::getDefaultProperties to load the Kernel's class properties as an array, grab the middleware group, and add the middleware to that. Then, I'd basically implode the array, and inject it into a Kernel.stub file, followed by overwriting the result to the application's Kernel.php

While this would work just fine, all comments or middleware that's included but 'commented' by default would be stripped out, and without a good way to grab those I think the end result would be less useful compared to what we have now.