-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Amended changes from #7452 #11897
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
Amended changes from #7452 #11897
Changes from all commits
35c1fca
8e0b778
09b83c9
8f7fcb3
c25e76b
eff333d
6e30cb2
cd91862
32321bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,20 +9,24 @@ | |
using System; | ||
using System.IO; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text.RegularExpressions; | ||
using System.Threading.Tasks; | ||
using Microsoft.AspNetCore.SpaServices.Extensions.Util; | ||
|
||
namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer | ||
namespace Microsoft.AspNetCore.SpaServices.DevelopmentServer | ||
{ | ||
internal static class ReactDevelopmentServerMiddleware | ||
internal static class DevelopmentServerMiddleware | ||
{ | ||
private const string LogCategoryName = "Microsoft.AspNetCore.SpaServices"; | ||
private static TimeSpan RegexMatchTimeout = TimeSpan.FromSeconds(5); // This is a development-time only feature, so a very long timeout is fine | ||
|
||
public static void Attach( | ||
ISpaBuilder spaBuilder, | ||
string npmScriptName) | ||
string npmScriptName, | ||
string waitText, | ||
Dictionary<string, string> extraArgs, | ||
string serverName = "App") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does there have to be a default value for this parameter? It would be clearer to make it mandatory if that's possible. |
||
{ | ||
var sourcePath = spaBuilder.Options.SourcePath; | ||
if (string.IsNullOrEmpty(sourcePath)) | ||
|
@@ -38,7 +42,7 @@ public static void Attach( | |
// Start create-react-app and attach to middleware pipeline | ||
var appBuilder = spaBuilder.ApplicationBuilder; | ||
var logger = LoggerFinder.GetOrCreateLogger(appBuilder, LogCategoryName); | ||
var portTask = StartCreateReactAppServerAsync(sourcePath, npmScriptName, logger); | ||
var portTask = StartDevServerAsync(sourcePath, npmScriptName, waitText, serverName, logger, extraArgs); | ||
|
||
// Everything we proxy is hardcoded to target http://localhost because: | ||
// - the requests are always from the local machine (we're not accepting remote | ||
|
@@ -54,43 +58,47 @@ public static void Attach( | |
// the first request times out, subsequent requests could still work. | ||
var timeout = spaBuilder.Options.StartupTimeout; | ||
return targetUriTask.WithTimeout(timeout, | ||
$"The create-react-app server did not start listening for requests " + | ||
$"The {serverName} server did not start listening for requests " + | ||
$"within the timeout period of {timeout.Seconds} seconds. " + | ||
$"Check the log output for error information."); | ||
}); | ||
} | ||
|
||
private static async Task<int> StartCreateReactAppServerAsync( | ||
string sourcePath, string npmScriptName, ILogger logger) | ||
private static async Task<int> StartDevServerAsync( | ||
string sourcePath, string npmScriptName, string waitText, string serverName, ILogger logger, Dictionary<string, string> extraArgs = null) | ||
{ | ||
var portNumber = TcpPortFinder.FindAvailablePort(); | ||
logger.LogInformation($"Starting create-react-app server on port {portNumber}..."); | ||
logger.LogInformation($"Starting {serverName} server on port {portNumber}..."); | ||
|
||
var envVars = new Dictionary<string, string> | ||
{ | ||
{ "PORT", portNumber.ToString() }, | ||
{ "BROWSER", "none" }, // We don't want create-react-app to open its own extra browser window pointing to the internal dev server port | ||
{ "PORT", portNumber.ToString() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's problematic to assume that all SPA development servers would use an environment variable to configure this, or that they would be looking for an environment variable with this particular name. |
||
}; | ||
|
||
waitText = waitText.Replace("$PORT", portNumber.ToString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a special custom syntax to define the message as a function of the port number is very nonstandard and makes me think we should reconsider this layering. |
||
|
||
extraArgs = extraArgs.ToDictionary(kvp => kvp.Key, kvp => kvp.Value.Replace("$PORT", portNumber.ToString())); | ||
|
||
var npmScriptRunner = new NpmScriptRunner( | ||
sourcePath, npmScriptName, null, envVars); | ||
sourcePath, npmScriptName, string.Join(" ", extraArgs.Select(x => x.Key + " " + x.Value).ToArray()), envVars); | ||
npmScriptRunner.AttachToLogger(logger); | ||
|
||
using (var stdErrReader = new EventedStreamStringReader(npmScriptRunner.StdErr)) | ||
{ | ||
try | ||
{ | ||
// Although the React dev server may eventually tell us the URL it's listening on, | ||
// Although the dev server may eventually tell us the URL it's listening on, | ||
// it doesn't do so until it's finished compiling, and even then only if there were | ||
// no compiler warnings. So instead of waiting for that, consider it ready as soon | ||
// as it starts listening for requests. | ||
await npmScriptRunner.StdOut.WaitForMatch( | ||
new Regex("Starting the development server", RegexOptions.None, RegexMatchTimeout)); | ||
new Regex(waitText, RegexOptions.None, RegexMatchTimeout)); | ||
} | ||
catch (EndOfStreamException ex) | ||
{ | ||
throw new InvalidOperationException( | ||
$"The NPM script '{npmScriptName}' exited without indicating that the " + | ||
$"create-react-app server was listening for requests. The error output was: " + | ||
$"{serverName} server was listening for requests. The error output was: " + | ||
$"{stdErrReader.ReadAsString()}", ex); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using Microsoft.AspNetCore.Builder; | ||
using System; | ||
using System.Collections.Generic; | ||
|
||
namespace Microsoft.AspNetCore.SpaServices.DevelopmentServer | ||
{ | ||
/// <summary> | ||
/// Extension methods for enabling React development server middleware support. | ||
/// </summary> | ||
public static class DevelopmentServerMiddlewareExtensions | ||
{ | ||
/// <summary> | ||
/// Handles requests by passing them through to an instance of a development npm web server. | ||
/// This means you can always serve up-to-date CLI-built resources without having | ||
/// to run the npm web server manually. | ||
/// | ||
/// This feature should only be used in development. For production deployments, be | ||
/// sure not to enable the npm web server. | ||
/// </summary> | ||
/// <param name="spaBuilder">The <see cref="ISpaBuilder"/>.</param> | ||
/// <param name="npmScript">The name of the script in your package.json file that launches the web server.</param> | ||
/// <param name="waitText">The text snippet identified during the build to indicate the Development Server has compiled and is ready.</param> | ||
/// <param name="serverName">The name of the Server used in the Console.</param> | ||
public static void UseDevelopmentServer( | ||
this ISpaBuilder spaBuilder, | ||
string npmScript, | ||
string waitText, | ||
string serverName = "App", | ||
Dictionary<string, string> extraArgs = null) | ||
{ | ||
|
||
if (string.IsNullOrEmpty(waitText)) | ||
{ | ||
throw new InvalidOperationException($"To use {nameof(UseDevelopmentServer)}, you must supply a non-empty value for the {nameof(waitText)} parameter. This allows us the find when the Development Server has started."); | ||
} | ||
|
||
if (spaBuilder == null) | ||
{ | ||
throw new ArgumentNullException(nameof(spaBuilder)); | ||
} | ||
|
||
var spaOptions = spaBuilder.Options; | ||
|
||
if (string.IsNullOrEmpty(spaOptions.SourcePath)) | ||
{ | ||
throw new InvalidOperationException($"To use {nameof(UseDevelopmentServer)}, you must supply a non-empty value for the {nameof(SpaOptions.SourcePath)} property of {nameof(SpaOptions)} when calling {nameof(SpaApplicationBuilderExtensions.UseSpa)}."); | ||
} | ||
|
||
DevelopmentServerMiddleware.Attach(spaBuilder, npmScript, waitText, extraArgs, serverName); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the file path and filename should be updated to match this.