Feature/stevesa/spaservices extensions package #1367

SteveSandersonMS · 2017-11-01T12:15:07Z

Our next update to the Angular template changes it substantially so that it's entirely based around Angular CLI. This much better matches the desired usage patterns. The community has welcomed this proposed change. We will very likely do the same thing for the React template, basing an updated template around the create-react-app tool.

The core implementation idea is to stop coupling the SPA APIs to MVC concepts (e.g., before, the routing was integrated with MVC routing, and prerendering was implemented as a tag helper), and instead have a set of middleware APIs that handle:

serving the SPA default page (e.g., index.html), thereby making client-side routing work
launching a development server (e.g., Angular CLI) and passing through requests to it
performing server-side prerendering of complete HTML pages

All of these new middleware APIs work equally well in MVC applications as in Razor Pages applications. For an example of their usage, please see https://github.com/aspnet/templating/blob/feature/stevesa/angular-cli-template/src/Microsoft.DotNet.Web.Spa.ProjectTemplates/content/Angular-CSharp/Startup.cs#L43

To support all this, we need some new runtime pieces. To allow for the possibility of shipping this out-of-band before the 2.1 release, 100% of the new runtime code is in a new package, Microsoft.AspNetCore.SpaServices.Extensions. This can ship with a dependency on SpaServices version 2.0.0, without any loss of back-compatibility (since SpaServices itself doesn't need to change). Later, we'll migrate the new functionality into SpaServices, and reduce SpaServices.Extensions to a set of type forwarders that exists only for back-compatibility (and at that point, the templates will no longer reference it for new applications).

As you'll see, the majority of code here is purely internal so we reserve the option to change implementation details pretty broadly once we get some preview feedback (and even after RTM). So for this review, I'm mostly keen on getting feedback on the public API design choices, though of course feel free to review whatever aspects of it you want.

SteveSandersonMS · 2017-11-03T16:21:40Z

Following @Eilon's feedback, I've reintroduced the ISpaBuilder concept. Out-of-the-box usage is now: https://github.com/aspnet/templating/blob/feature/stevesa/angular-cli-template/src/Microsoft.DotNet.Web.Spa.ProjectTemplates/content/Angular-CSharp/Startup.cs#L43

The difference is that instead of attaching extra SPA middleware to the IApplicationBuilder app variable captured from the enclosing scope, the developer now attaches SPA middleware to a new ISpaBuilder variable that is passed as a parameter to the configuration callback, i.e., you have app.UseSpa(spa => { ... }) and then perform further config on spa instead of on app.

This more closely parallels how other nested-config patterns work in ASP.NET (e.g., app.Map(childApp => { ... }) or app.UseMvc(routes => { ... })).

A further consequence is that it's more constraining about how other SPA-related middleware can be used. It's now not possible to use middleware like UseProxyToSpaDevelopmentServer directly on an IApplicationBuilder - you now can only use it within the context of a UseSpa configuration callback. This is probably an improvement, since (1) this was the intended usage pattern anyway, and now there are fewer combinations of usages to reason about, and (2) it's easier to discover the available SPA-related middleware via intellisense - it just pops up when you type spa. inside the configuration callback, without being lost among a hundred other middleware APIs.

jrmcdona · 2017-11-05T12:30:02Z

@SteveSanderson is there a simple way to load the new template in beta or preview? I am setting up a new project and have some time on my side to test your template out

jxvalenz · 2017-11-06T01:27:13Z

Same here. I’m setting up a new POC project and I would love to start testing the new template with angular-cli

Eilon

Overall looks good. Let's chat about a few of my comments in person.

Eilon · 2017-11-06T17:58:20Z

src/Microsoft.AspNetCore.SpaServices.Extensions/DefaultSpaBuilder.cs

+        public DefaultSpaBuilder(IApplicationBuilder applicationBuilder, string sourcePath, string urlPrefix)
+        {
+            ApplicationBuilder = applicationBuilder 
+                ?? throw new System.ArgumentNullException(nameof(applicationBuilder));


Do you use using, bro?

Eilon · 2017-11-06T18:04:44Z

src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliMiddleware.cs

+                try
+                {
+                    openBrowserLine = await npmScriptRunner.StdOut.WaitForMatch(
+                        new Regex("open your browser on (http\\S+)"),


Regex? We should discuss. 😭

TODO: Add timeout to the regex

Eilon · 2017-11-06T18:07:04Z

src/Microsoft.AspNetCore.SpaServices.Extensions/AngularCli/AngularCliMiddleware.cs

+            }
+        }
+
+#pragma warning disable CS0649


Hmm why would this warning come up?

Field 'field' is never assigned to, and will always have its default value 'value'
The compiler detected an uninitialized private or internal field declaration that is never assigned a value.

Is Port never read? Or is it read via reflection somewhere (e.g. via serialization)?

If that's the case, should add a comment stating that.

TODO: Add comment clarifying what CS0649 means
TODO: Add comment saying why it's used here

Eilon · 2017-11-06T18:52:44Z

src/Microsoft.AspNetCore.SpaServices.Extensions/Prerendering/SpaPrerenderingExtensions.cs

+        public static void UseSpaPrerendering(
+            this ISpaBuilder spaBuilder,
+            string entryPoint,
+            ISpaPrerendererBuilder buildOnDemand = null,


This is a lot of optional params, which can cause major headaches if we ever need to add new parameters in terms of breaking changes (not impossible, just difficult and ugly). Any concerns here?

TODO: Change UseSpaPrerendering to take an options object callback

Eilon · 2017-11-06T19:29:17Z

src/Microsoft.AspNetCore.SpaServices.Extensions/SpaOptions.cs

+        /// <summary>
+        /// Gets or sets the URL, relative to <see cref="UrlPrefix"/>,
+        /// of the default page that hosts your SPA user interface.
+        /// The typical value is <c>"index.html"</c>.


Instead of typical maybe just say it's the default?

SteveSandersonMS · 2017-11-07T00:00:09Z

TODO: On UseSpa, remove the defaultPage optional param
Update: done.

davidfowl · 2017-11-07T07:21:33Z

src/Microsoft.AspNetCore.SpaServices.Extensions/Npm/EventedStreamReader.cs

+            var buf = new char[8 * 1024];
+            while (true)
+            {
+                var chunkLength = await _streamReader.ReadAsync(buf, 0, buf.Length);


Why not use ReadLineAsync?

Because we want notifications of both partial-line chunks (for progress updates) and complete lines (that can be forwarded to the ILogger).

…ew runtime functionality needed for updated templates until 2.1 ships

…endencies" because it breaks the build. Will need to find a different way to enforce this. This reverts commit 105422b.

…the app is shutting down

…ly it to the boot function

…o go via NodeServices)

…of AngularCliMiddleware

…affect public API.

…ry to keep restarting Angular CLI etc. on C# changes

…onditional' any more. This is internal, so the name change is fine.

…instead of optional params

…ld basically always need to use it

Eilon

Code changes look good to me. Let's review the API/usage this afternoon.

SteveSandersonMS · 2017-11-07T23:33:08Z

Further CR feedback:

Move UseWebsockets into UseAngularCliServer/UseProxyToSpaDevelopmentServer
Strip out console colours from the Angular CLI stdio piping
Support IOptions`1[SpaOptions]
Change urlPrefix to be a PathString
Change UseSpa to take only an Action, and set the UrlPrefix/SourcePath from inside the callback (making them settable)
Remove the UseSpaPrerendering comment block from the template
Add overload to UseProxyToSpaDevelopmentServer that takes a string for baseUri

…ts support

…fix/SourcePath from inside the callback

… string for the URI

Eilon

The updated codes continue to look good. Awaiting the final updates for final actual sign-off.

SteveSandersonMS · 2017-11-08T23:22:40Z

@Eilon Should be ready for final approval now

…FileProvider to support SPAs not served from wwwroot (e.g., React).

Eilon

Final-approved.

SteveSandersonMS · 2017-11-09T18:19:39Z

@Eilon Made a few final tweaks before merging. Will chat when I spot you around.

Eilon · 2017-11-09T18:25:45Z

src/Microsoft.AspNetCore.SpaServices.Extensions/SpaOptions.cs

                {
-                    throw new ArgumentException($"The value for {nameof(DefaultPage)} cannot be null or empty.");
+                    throw new ArgumentNullException($"The value for {nameof(DefaultPage)} cannot be null or empty.");


Change back to ArgumentException - this is also thrown for empty strings.

Well spotted. Done.

SpeedHighway · 2017-12-18T19:04:59Z

This line:
openBrowserLine = await npmScriptRunner.StdOut.WaitForMatch( new Regex("open your browser on (http\\S+)", RegexOptions.None, RegexMatchTimeout), StartupTimeout);

Is causing my site to fail every time, unless I manually stop the browser quickly, after pressing F5 (or Ctrl F5) in Visual Studio

Because of the fact that the console window appears and states:

** NG Live Development Server is listening on localhost:5155, open your browser on http://localhost:5155/ **

The browser opens immediately, as it starts to do the build. However, the build definitely does not finish in 50 seconds, so then it times out (displaying an error while building) and gets stuck unusable due to hitting the exception for the timeout.

The browser should not be opening up until the webpack: Compiled successfully. appears, unless I'm mistaken.

Thanks

Eilon · 2017-12-18T22:38:22Z

@SpeedHighway can you log this in a new issue? That'll make it easier for us to track and investigate.

dnfclas added the cla-already-signed label Nov 1, 2017

SteveSandersonMS requested review from Eilon and davidfowl November 1, 2017 12:15

Eilon suggested changes Nov 6, 2017

View reviewed changes

davidfowl reviewed Nov 7, 2017

View reviewed changes

SteveSandersonMS added 21 commits November 7, 2017 09:07

Add new Microsoft.AspNetCore.SpaServices.Extensions package to host n…

7682c74

…ew runtime functionality needed for updated templates until 2.1 ships

SpaServices.Extensions will first ship to work with 2.0.0 dependencies

415aa54

Revert "SpaServices.Extensions will first ship to work with 2.0.0 dep…

77fcfc2

…endencies" because it breaks the build. Will need to find a different way to enforce this. This reverts commit 105422b.

Make ConditionalProxy shut down the WebSocket proxy much faster when …

257e095

…the app is shutting down

Make UseSpaPrerendering capture the non-prerendered response and supp…

2485911

…ly it to the boot function

Add API for supplying custom data to prerenderer

eff9bb9

Remove angular-cli-middleware's dependency on Promise

2404cc2

Better handle errors during prerendering

e4e396e

Capture errors and timeouts that occur ing angular-cli-middleware.js

0e1bde4

Have AngularCliMiddleware run npm scripts directly (no longer needs t…

9ee8cbc

…o go via NodeServices)

Add ISpaOptions concept so that AngularCliBuilder can be independent …

08dbc15

…of AngularCliMiddleware

Better logging if an NPM task exits with an error

ec21464

Simplify AngularCliMiddleware by reducing to a static class. Doesn't …

272e609

…affect public API.

Add standalone UseProxyToSpaDevelopmentServer API so it's not necessa…

ae2c456

…ry to keep restarting Angular CLI etc. on C# changes

Simplify 404 handling in new SPA proxying code

10a114c

Rename the new ConditionalProxy to SpaProxy, since it's not always 'c…

3c313ac

…onditional' any more. This is internal, so the name change is fine.

Code clean-ups

f9f365b

Following CR feedback, reintroduce ISpaBuilder concept

99a5611

CR feedback: Clean up timeouts

0849d45

Update project reference to match new KoreBuild requirements

d2f7f83

Remove redundant warning suppression

e34c261

SteveSandersonMS added 4 commits November 7, 2017 09:15

Add 'using'

82507eb

CR feedback: Switch to configuration callback for UseSpaPrerendering …

a93e1a1

…instead of optional params

CR feedback: Change comment from 'typical' to 'default'

bb10e56

CR feedback: Remove vestigal 'defaultPage' param

eb403ed

SteveSandersonMS force-pushed the feature/stevesa/spaservices-extensions-package branch from 87eb919 to eb403ed Compare November 7, 2017 17:55

On UseSpaPrerendering, make 'configuration' mandatory because you wou…

eb04c98

…ld basically always need to use it

Eilon approved these changes Nov 7, 2017

View reviewed changes

SteveSandersonMS added 6 commits November 7, 2017 15:41

Make UseProxyToSpaDevelopmentServer responsible for enabling WebSocke…

974ace4

…ts support

CR feedback: Change UseSpa to take only an Action, and set the UrlPre…

dc7f14a

…fix/SourcePath from inside the callback

CR feedback: Change UrlPrefix property to be a PathString

ca37e91

CR feedback: Add UseProxyToSpaDevelopmentServer overload that takes a…

54cea3d

… string for the URI

CR feedback: Use IOptions pattern

ca7ae03

Additional changes that should have been included in previous commit

a5bd320

Eilon approved these changes Nov 8, 2017

View reviewed changes

CR feedback: Strip ANSI colours when writing to ILogger

b5b0356

SteveSandersonMS added 2 commits November 8, 2017 18:11

Eliminate UrlPrefix because it's no longer much used. Add DefaultPage…

31af70f

…FileProvider to support SPAs not served from wwwroot (e.g., React).

Fix XML doc

9b2e279

Eilon approved these changes Nov 9, 2017

View reviewed changes

Move UseSpaPrerendering entryPoint config onto SpaPrerenderingOptions

073fc7a

SteveSandersonMS closed this Nov 9, 2017

SteveSandersonMS deleted the feature/stevesa/spaservices-extensions-package branch November 9, 2017 18:14

Eilon reviewed Nov 9, 2017

View reviewed changes

SpeedHighway mentioned this pull request Dec 18, 2017

Angular-CLI call is starting browser too soon and causing timeout #1447

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/stevesa/spaservices extensions package #1367

Feature/stevesa/spaservices extensions package #1367

SteveSandersonMS commented Nov 1, 2017

SteveSandersonMS commented Nov 3, 2017 •

edited

Loading

jrmcdona commented Nov 5, 2017

jxvalenz commented Nov 6, 2017

Eilon left a comment

Eilon Nov 6, 2017

SteveSandersonMS Nov 6, 2017

SteveSandersonMS Nov 7, 2017

Eilon Nov 6, 2017

SteveSandersonMS Nov 6, 2017

SteveSandersonMS Nov 7, 2017

Eilon Nov 6, 2017

SteveSandersonMS Nov 6, 2017

SteveSandersonMS Nov 7, 2017

Eilon Nov 6, 2017

SteveSandersonMS Nov 6, 2017

SteveSandersonMS Nov 7, 2017

Eilon Nov 6, 2017

SteveSandersonMS Nov 7, 2017

SteveSandersonMS commented Nov 7, 2017 •

edited

Loading

davidfowl Nov 7, 2017

SteveSandersonMS Nov 7, 2017

Eilon left a comment

SteveSandersonMS commented Nov 7, 2017 •

edited

Loading

Eilon left a comment

SteveSandersonMS commented Nov 8, 2017

Eilon left a comment

SteveSandersonMS commented Nov 9, 2017

Eilon Nov 9, 2017

SteveSandersonMS Nov 9, 2017

SpeedHighway commented Dec 18, 2017

Eilon commented Dec 18, 2017

Feature/stevesa/spaservices extensions package #1367

Feature/stevesa/spaservices extensions package #1367

Conversation

SteveSandersonMS commented Nov 1, 2017

SteveSandersonMS commented Nov 3, 2017 • edited Loading

jrmcdona commented Nov 5, 2017

jxvalenz commented Nov 6, 2017

Eilon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eilon left a comment

Choose a reason for hiding this comment

SteveSandersonMS commented Nov 7, 2017 • edited Loading

Eilon left a comment

Choose a reason for hiding this comment

SteveSandersonMS commented Nov 8, 2017

Eilon left a comment

Choose a reason for hiding this comment

SteveSandersonMS commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpeedHighway commented Dec 18, 2017

Eilon commented Dec 18, 2017

SteveSandersonMS commented Nov 3, 2017 •

edited

Loading

SteveSandersonMS commented Nov 7, 2017 •

edited

Loading

SteveSandersonMS commented Nov 7, 2017 •

edited

Loading