[Templates] SPA template proxy approach update

[javiercn]

• Introduces a new package Microsoft.AspNetCore.SpaProxy to handle launching the SPA proxy as the frontend for the app.
• Removes unnecessary references to Microsoft.AspNetCore.SpaExtensions.
• Updates the angular template to use a new approach where the SPA frontend proxy forwards requests to the backend API.

Additional details are here

TL;DR
There is a new package responsible for launching an NPM command to start the proxy upon startup.
The proxy launches using https in 5002 and forwards requests to 5001 for the configured paths.
Users can run from VS with a single F5 gesture (same experience).
Users can run with dotnet run with a single gesture.
Users can start the proxy manually with npm start and then do dotnet run separately.

[pranavkm]

Sort usings

[pranavkm]

Suggested change

	bool running = await ProbeSpaDevelopmentServerUrl(cancellationToken);
	var running = await ProbeSpaDevelopmentServerUrl(cancellationToken);

[pranavkm]

This should use the LoggerMessage pattern

[javiercn]

This is only used for tooling purposes at development time, the LoggerMessage pattern is a bit overkill for this case.

[pranavkm]

Could you write a comment about what it's doing here? This is the value of LaunchCommand, this is what we're trying to do etc?

[SteveSandersonMS]

We might also want to note that we're deliberately not supporting LaunchCommand executable paths that contain spaces (for example: "c:\Users\My User\nodejs\bin\npm.cmd", which might seem valid given the surrounding quotes).

Somebody out there is going to run into trouble with spaces but it's probably enough of an edge case not to address if it requires bringing in some more complex parsing logic, especially given how you've made it easy for them to launch the SPA dev server manually if they want.

[javiercn]

This only needs to support yarn/npm <<verb>> <<args>> if you want something very specialize then you can always wrap it on a shell script and use that as your command.

[javiercn]

I added a comment to make it clear.

[pranavkm]

Might want to check if the process hasn't already exited. But this seems like a reasonable thing to do.

[SteveSandersonMS]

I honestly have no idea what combination of behaviors might occur for different SPA dev servers and OSes in which shutdown scenarios. From memory, it's very tricky trying to get consistent behaviors for shutting down child processes.

In all likelihood we'll just have to see what happens in practice with the React/Angular/etc dev servers on different OSes and whether it leads to a workflow people find convenient. We can tweak the shutdown logic later if there are any problematic cases, such as something being left half-shut-down.

[javiercn]

All the frameworks use the webpack dev server in the end. From my empirical tests CloseMainWindow() tries to do the right thing or returns false (but conveniently doesn't throw).

The behavior here is at least comparable to the behavior in the script runner SpaExtensions, I verified the shutdown across OSes on common scenarios (ctrl+c, close window, etc.). We might end up not even shipping this package in RTM, so I think it's good for now (or at least as good as its going to get)

[pranavkm]

Probably want to dispose _spaProcess too

[javiercn]

This have made me think that this code might not be 100% correct, since I'm not 100% sure I should mock with the _spaProcess instance when running in the context of the finalizer. That said, I don't think that matters since this object is managed by the DI container and will always be disposed, and the worst case scenario is an that an exception is thrown and caught within the finalizer, which I think it's fine.

I'm not going to change it since that would require me to retest all the templates across OSs again.

[pranavkm]

Suggested change

	if(!certificateName){
	if (!certificateName) {

[pranavkm]

sort usings

[SteveSandersonMS]

This is a fairly minor point as it would only matter potentially in the future but... it feels kinda dangerous to have this _httpClient lying around in this state, as someone might start using it for other purposes in the future where they actually need cert validation to happen as usual.

Since this HttpClient instance is only used to probe for the SPA server startup, it might be nice not to have it as a class-level field, and instead new up an instance each time we're going to do a set of calls into ProbeSpaDevelopmentServerUrl. The perf cost of doing that would be nothing in practice.

[SteveSandersonMS]

How long might this call last? Do we generally find that the SPA dev servers reply instantly if they are alive, or do they potentially wait minutes while going through a webpack build?

Maybe even if they do take ages it's desirable to wait for them. Just asking to clear up my understanding of the startup flow.

[javiercn]

It comes back right away. However I've updated the code to add a timeout after 1 second or if the host request cancellation, whatever happens first.

[SteveSandersonMS]

Looks like the !livenessProbeSucceeded part of this condition isn't needed, because it only becomes true on L79, after which we'd break out of the loop anyway.

Suggested change

	while (_spaProcess != null && !_spaProcess.HasExited && !livenessProbeSucceeded && !maxTimeoutReached)
	while (_spaProcess != null && !_spaProcess.HasExited && !maxTimeoutReached)

[SteveSandersonMS]

Should we impose a shorter timeout here than whatever cancellationToken has? Since we're polling, it would seem more natural to only wait a few seconds at a time, rather than potentially waiting for minutes on each loop.

The MaxTimeout will only be honored with reasonable accuracy if each polling loop is short enough.

[javiercn]

Each poll attempt will wait at most 1 second to receive an answer and 1 second after each unsuccessful connection

[SteveSandersonMS]

This is an interesting design. TBH I was expecting developers to create and edit the spa.proxy.json file themselves - I didn't predict we'd be generating it at build time based on a set of MSBuild properties.

One drawback to generating it like this is with formatting. The logic here will produce an invalid JSON file if SpaProxyLaunchCommand contains a " character, which could actually be very common since people like to pass all kinds of arguments to their startup commands. Not sure if \n or other characters would cause issues too.

Do you think the MSBuild properties are more discoverable/convenient than just telling people to drop in a spa.proxy.json file to their project? In both cases they will only know about this by reading some docs or tutorial, so in either design we're relying on them copy/pasting stuff.

[javiercn]

One drawback to generating it like this is with formatting. The logic here will produce an invalid JSON file if SpaProxyLaunchCommand contains a " character, which could actually be very common since people like to pass all kinds of arguments to their startup commands. Not sure if \n or other characters would cause issues too.

In general you should put arguments inside package.json otherwise you loose the ability to run npm/yarn start from the cli. I started down that route where the command in the project file contained everything. There are alternatives to make this work for all people if needed, like wrapping the command in a script file and using the script for launch instead.

[SteveSandersonMS]

By all means please ignore this suggestion if you don't like it, but the code might seem less intimidating and easier to skim-read if formatted closer to the JS-typical brace style and more compactly:

Suggested change

	spawn(
	'dotnet',
	[
	'dev-certs',
	'https',
	'--export-path',
	certFilePath,
	'--format',
	'Pem',
	'--no-password',
	],
	{
	stdio: 'inherit',
	}
	).on('exit', (code) => process.exit(code));
	spawn('dotnet', [
	'dev-certs', 'https',
	'--no-password',
	'--export-path', certFilePath,
	'--format', 'Pem',
	], { stdio: 'inherit' })
	.on('exit', code => process.exit(code));

[javiercn]

vs code formatting is fighting me :). I'll fix it, it used to be that way

[SteveSandersonMS]

Some slightly funky formatting going on here. Also if you're interested, this can be done without reduce which would reduce the scariness factor a bit:

Suggested change

	const certificateName = process.argv.map(arg => arg.match(/--name=(?<value>.+)/i))
	.filter(Boolean)
	.reduce((previous, current) => previous || current.groups.value, undefined) ||
	process.env.npm_package_name;
	const certificateArg = process.argv.map(arg => arg.match(/--name=(?<value>.+)/i)).filter(Boolean)[0];
	const certificateName = certificateArg ? certificateArg.groups.value : process.env.npm_package_name;

The ternary could be reduced further to certificateArg?.groups.value || process.env.npm_package_name but I think that's harder to understand.

Since this is just code style please feel free to ignore the suggestion!

[javiercn]

?. and stuff like that is not a good idea in these script files since they need to run in node, but I get your point.

I didn't know you could index on an empty array in JS, that's tricky but neat!

[SteveSandersonMS]

Well, this file looks a lot nicer now! Great stuff!

[SteveSandersonMS]

This looks superb to me. I'm impressed you were able to find ways of bringing so much functionality while hardly increasing the conceptual load or implementation complexity at all.

[davidfowl]

Why?

[javiercn]

This is a development time only package that we wire-up in launchSettings.json and that takes care of launching the angular/react proxy server when you do dotnet run it's not used in the published app.

[javiercn]

You can find all the details here

[davidfowl]

Why is there a finalizer?

[javiercn]

To ensure cleanup of the process in as many situations as possible. This is a development time only package