Skip to content
This repository was archived by the owner on Apr 8, 2020. It is now read-only.

Added OnBeforeStartExternalProcess callback #177

Closed
wants to merge 1 commit into from

Conversation

thunder7553
Copy link
Contributor

Added OnBeforeStartExternalProcess callback which to NodeServicesOptions (and OutOfProcessNodeInstance, SocketNodeInstance and HttpNodeInstance) to configure environment of the node.exe process to be started, and the path to the node executable itself. Fixes #20

…ons (and OutOfProcessNodeInstance, SocketNodeInstance and HttpNodeInstance) to configure environment of the node.exe process to be started, and the path to the node executable itself. Fixes aspnet#20
@dnfclas
Copy link

dnfclas commented Jul 13, 2016

Hi @thunder7553, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Jul 13, 2016

@thunder7553, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@SteveSandersonMS
Copy link
Member

Thanks for this! I've now merged it. I hope you don't mind, but I've also changed the API.

I was not keen to expand the set of constructor parameters for all the hosting models (even if it's a new optional parameter, it's still something that everyone calling the constructor has to see and figure out the relevance of).

But since it was a constructor parameter, the only way to make use of it would have been when calling new directly (and not just when receiving an INodeInstance through DI). So if a developer is already doing that, it's hardly any extra work to define a subclass of whatever hosting model they are instantiating. So, this can conveniently be done as a virtual method instead of a constructor parameter.

You can now do this:

public class MyHttpNodeInstance : HttpNodeInstance
{
    protected override ProcessStartInfo PrepareNodeProcessStartInfo(string entryPointFilename, string projectPath, string commandLineArguments)
    {
        var startInfo = base.PrepareNodeProcessStartInfo(entryPointFilename, projectPath, commandLineArguments);
        // Now modify 'startInfo' however you want to here
        return startInfo;
    }
}

I know this is slightly more code than would have been needed with the constructor param, but it has the benefit of not interjecting an extra concept to think about for developers who are just trying to instantiate a node instance without wanting to override the process start info in any way. Hope that's OK - let me know if you disagree!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants