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

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

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

RobertBoes
Copy link
Contributor

@RobertBoes RobertBoes commented Jan 15, 2023

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
Copy link

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

@RobertBoes
Copy link
Contributor Author

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

@RobertBoes RobertBoes changed the title Add pcntl to requirements Add pcntl to composer suggestions Jan 16, 2023
@rojtjo
Copy link
Contributor

rojtjo commented Jan 16, 2023

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

@darthsoup
Copy link

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

@rojtjo
Copy link
Contributor

rojtjo commented Jan 16, 2023

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
Copy link
Contributor Author

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
Copy link
Member

reinink commented Jan 17, 2023

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.

@reinink reinink merged commit b983c6e into inertiajs:master Jan 17, 2023
@reinink reinink changed the title Add pcntl to composer suggestions Conditionally use pcntl extension in inertia:start-ssr command Jan 17, 2023
@RobertBoes RobertBoes deleted the pcntl branch January 17, 2023 06:13
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.

4 participants