inertiajs / inertia-laravel

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

Conditionally use `pcntl` extension in `inertia:start-ssr` command #492

Closed RobertBoes closed 1 year ago

RobertBoes commented 1 year ago

v0.6.6 introduced artisan commands to start the SSR server. Later this was updated (#487), but the code introduced there uses pcntl, which isn't currently part of the requirements. This adds ext-pcntl to the requirements. Since the docs now only provide information to start the SSR server through this command I feel like this requirement should be added to Inertia.

One side-effect of this is that's it's impossible to use this feature on systems where the extension isn't available, like Windows.

darthsoup commented 1 year ago

It might be better to just include it in the suggestions. Not everyone uses SSR.

RobertBoes commented 1 year ago

@darthsoup Yeah you're right. I moved it to suggest and added a check to the command, that seems like a better solution, thanks 👍

rojtjo commented 1 year ago

Is pcntl actually required to run SSR or is it only required for graceful shutdowns?

darthsoup commented 1 year ago

@rojtjo depends on the distribution and the operating system. pcntl does not work under Windows

rojtjo commented 1 year ago

I know it's not available on Windows, but was just wondering if it's even required to start the SSR server.

Looking at the code it's only used to register a stop handler which kills the SSR process, but as far as I know the SSR process will also be automatically killed when the parent process (artisan inertia:start-ssr) stops. This would make the pcntl actually optional and should not prevent this command from running.

RobertBoes commented 1 year ago

That's probably right, could also just wrap the pcntl calls in an if-statement when the extension is loaded.

I know it's not available on Windows, but was just wondering if it's even required to start the SSR server.

Yeah, the artisan command isn't required in the first place, it's only added for error reporting I believe. I've added this to my own implementation of the SSR gateway. I personally won't use this command, as I find the setup strange (a PHP process to start a node process). But, as the code is right now, it requires pcntl. Don't know if it's desirable to rewrite the code to call these methods optionally

reinink commented 1 year ago

Hey thanks for this @RobertBoes!

I've just tweaked it a little to conditionally use the pcntl extension when it's available, and I updated the suggest message to reflect that it's recommended when using the inertia:start-ssr command.

I think this should generally be fine, because even using the pcntl functions here was being "extra safe", basically just cleaning up any old node processes if things go bad.