-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Abstract ReactDevelopmentServerMiddleware to DevelopmentServerMiddleware for other NPM Web Servers #7452
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
Abstract ReactDevelopmentServerMiddleware to DevelopmentServerMiddleware for other NPM Web Servers #7452
Changes from all commits
35c1fca
8e0b778
09b83c9
8f7fcb3
c25e76b
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 |
---|---|---|
@@ -0,0 +1,87 @@ | ||
// 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; | ||
|
||
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") | ||
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. To me, these APIs feel extremely over-specialized. What if the "ready" signal isn't some static text you can look for? What if the way to start it isn't running an NPM script? What I'd expect instead would be to decompose this into lower-level building blocks, e.g.:
What do you think - could this be an improvement? |
||
{ | ||
|
||
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, serverName); | ||
} | ||
|
||
/// <summary> | ||
/// Handles requests by passing them through to an instance of the create-react-app server. | ||
/// This means you can always serve up-to-date CLI-built resources without having | ||
/// to run the create-react-app server manually. | ||
/// | ||
/// This feature should only be used in development. For production deployments, be | ||
/// sure not to enable the create-react-app 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 create-react-app server.</param> | ||
public static void UseReactDevelopmentServer( | ||
this ISpaBuilder spaBuilder, | ||
string npmScript) | ||
{ | ||
UseDevelopmentServer(spaBuilder, npmScript, "Starting the development server", "create-react-app"); | ||
} | ||
|
||
/// <summary> | ||
/// Handles requests by passing them through to an instance of the vue-cli-service server. | ||
/// This means you can always serve up-to-date CLI-built resources without having | ||
/// to run the vue-cli-service server manually. | ||
/// | ||
/// This feature should only be used in development. For production deployments, be | ||
/// sure not to enable the vue-cli-service 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 vue-cli-service server.</param> | ||
public static void UseVueDevelopmentServer( | ||
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. I appreciate that this adds a feature, but we haven't announced any plan to build in support for Vue-specific things. Previously, we explicitly stepped away from doing that, so we could focus on the two most demanded third-party SPA frameworks (Angular and React). If we were to change this plan (and it might be that, for the time being, it doesn't change) that would be a decision for @danroth27. |
||
this ISpaBuilder spaBuilder, | ||
string npmScript) | ||
{ | ||
UseDevelopmentServer(spaBuilder, npmScript, "Starting development server...", "vue-cli-service"); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using Microsoft.AspNetCore.Builder; | ||
using Microsoft.AspNetCore.SpaServices.DevelopmentServer; | ||
using System; | ||
|
||
namespace Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer | ||
|
@@ -21,23 +22,12 @@ public static class ReactDevelopmentServerMiddlewareExtensions | |
/// </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 create-react-app server.</param> | ||
[Obsolete("Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer.ReactDevelopmentServerMiddlewareExtensions.UseReactDevelopmentServer is deprecated, please use Microsoft.AspNetCore.SpaServices.DevelopmentServer.DevelopmentServerMiddlewareExtensions.UseReactDevelopmentServer instead.")] | ||
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. Rather than obsoleting this, could the new logic go here instead of making a new extension method for it? |
||
public static void UseReactDevelopmentServer( | ||
this ISpaBuilder spaBuilder, | ||
string npmScript) | ||
{ | ||
if (spaBuilder == null) | ||
{ | ||
throw new ArgumentNullException(nameof(spaBuilder)); | ||
} | ||
|
||
var spaOptions = spaBuilder.Options; | ||
|
||
if (string.IsNullOrEmpty(spaOptions.SourcePath)) | ||
{ | ||
throw new InvalidOperationException($"To use {nameof(UseReactDevelopmentServer)}, you must supply a non-empty value for the {nameof(SpaOptions.SourcePath)} property of {nameof(SpaOptions)} when calling {nameof(SpaApplicationBuilderExtensions.UseSpa)}."); | ||
} | ||
|
||
ReactDevelopmentServerMiddleware.Attach(spaBuilder, npmScript); | ||
DevelopmentServerMiddlewareExtensions.UseReactDevelopmentServer(spaBuilder, npmScript); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -477,14 +477,41 @@ | |
"GenericParameters": [] | ||
}, | ||
{ | ||
"Name": "Microsoft.AspNetCore.SpaServices.ReactDevelopmentServer.ReactDevelopmentServerMiddlewareExtensions", | ||
"Name": "Microsoft.AspNetCore.SpaServices.DevelopmentServer.DevelopmentServerMiddlewareExtensions", | ||
"Visibility": "Public", | ||
"Kind": "Class", | ||
"Abstract": true, | ||
"Static": true, | ||
"Sealed": true, | ||
"ImplementedInterfaces": [], | ||
"Members": [ | ||
{ | ||
"Kind": "Method", | ||
"Name": "UseDevelopmentServer", | ||
"Parameters": [ | ||
{ | ||
"Name": "spaBuilder", | ||
"Type": "Microsoft.AspNetCore.SpaServices.ISpaBuilder" | ||
}, | ||
{ | ||
"Name": "npmScript", | ||
"Type": "System.String" | ||
}, | ||
{ | ||
"Name": "waitText", | ||
"Type": "System.String" | ||
}, | ||
{ | ||
"Name": "serverName", | ||
"Type": "System.String" | ||
} | ||
], | ||
"ReturnType": "System.Void", | ||
"Static": true, | ||
"Extension": true, | ||
"Visibility": "Public", | ||
"GenericParameter": [] | ||
}, | ||
{ | ||
"Kind": "Method", | ||
"Name": "UseReactDevelopmentServer", | ||
|
@@ -503,6 +530,25 @@ | |
"Extension": true, | ||
"Visibility": "Public", | ||
"GenericParameter": [] | ||
}, | ||
{ | ||
"Kind": "Method", | ||
"Name": "UseVueDevelopmentServer", | ||
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. As per the comment above, this should only be added after an explicit product decision (which might not happen!) |
||
"Parameters": [ | ||
{ | ||
"Name": "spaBuilder", | ||
"Type": "Microsoft.AspNetCore.SpaServices.ISpaBuilder" | ||
}, | ||
{ | ||
"Name": "npmScript", | ||
"Type": "System.String" | ||
} | ||
], | ||
"ReturnType": "System.Void", | ||
"Static": true, | ||
"Extension": true, | ||
"Visibility": "Public", | ||
"GenericParameter": [] | ||
} | ||
], | ||
"GenericParameters": [] | ||
|
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.
I would have thought this shouldn't be here, as it's specific to React. Could the method take a dictionary of environment variables as a parameter instead?