Abstract ReactDevelopmentServerMiddleware to DevelopmentServerMiddleware for other NPM Web Servers #7452
Conversation
…dleware, obsoleted old namespace, Added Generic UseDevelopmentServer and Vue UseVueDevelopmentServer.
Tratcher
requested review from
SteveSandersonMS
and removed request for
Tratcher
Eilon
added
the
area-mvc
|
@SteveSandersonMS Any news? |
|
|
||
| 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 | ||
| { "BROWSER", "none" }, // We don't want the dev server to open its own extra browser window pointing to the internal dev server port |
There was a problem hiding this comment.
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?
| this ISpaBuilder spaBuilder, | ||
| string npmScript, | ||
| string waitText, | ||
| string serverName = "App") |
There was a problem hiding this comment.
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.:
- Some method for starting an NPM script and getting back the
Process - Some method for transforming the standard output stream from a
Processinto an async-enumerable-of-string UseReactDevelopmentServerandUseAngularClimethods that call the "start NPM task" method, then awaits until it outputs the expected "I'm running now" string. These can each contain their own instructions to output text like "Starting Angular CLI server...", so no need to make these into parameters on something else.
What do you think - could this be an improvement?
| /// </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.
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.
| @@ -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.
Rather than obsoleting this, could the new logic go here instead of making a new extension method for it?
| }, | ||
| { | ||
| "Kind": "Method", | ||
| "Name": "UseVueDevelopmentServer", |
There was a problem hiding this comment.
As per the comment above, this should only be added after an explicit product decision (which might not happen!)
There was a problem hiding this comment.
Hope you don't mind these suggestions!
@ryanbrandenburg, are you interested in taking over on this PR? No worries if not - I'm happy to continue the conversation here.
|
@SteveSandersonMS I won't have useful knowledge here until I've been brought up to speed on the whole area, so it's probably best if you stick with it for now. |
|
@SteveSandersonMS , @ryanbrandenburg what is pending here? |
|
A response to @SteveSandersonMS's request for changes. It'll also need to rebase to solve that conflict. |
|
I've also requested this feature before (#8024), would be happy to attempt to implement @SteveSandersonMS's changes? EDIT: #11897 |
- Removed VueDevelopmentServer - Added Dictionary for arguments to DevelopmentServer - Unobseleted ReactDevelopmentServer
|
Closing as per conversation in #11897 |
mkArtakMSFT
added
the
tell-mode
Summary of the changes (Less than 80 chars)
Abstracted ReactDevelopmentServerMiddleware into DevelopmentServerMiddleware, obsoleted old namespace, Added Generic UseDevelopmentServer and Vue UseVueDevelopmentServer.
This opens up SpaExtensions for Vue.js as well as other npm/webpack development servers for serving UI Frameworks.
This can be used to make a Vue.js Visual Studio Template.