Add a middleware for browser refresh. #24574

pranavkm · 2020-08-05T00:27:17Z

Introduce a middleware that can connect to the dotnet-watch change server
dotnet-watch: Inject the middleware in 3.1 or apps using start hooks \ hosting startup

BrennanConroy · 2020-08-05T17:33:35Z

Did you mean to put this in preview8?

Tactics Ask Mode - ship blocking only

* Introduce a middleware that can connect to the dotnet-watch change server * dotnet-watch: Inject the middleware in 3.1 or apps using start hooks \ hosting startup #23412

pranavkm · 2020-08-05T17:50:07Z

eng/Workarounds.targets

@@ -20,7 +20,7 @@
    The web sdk adds an implicit framework reference. This removes it until we can update our build to use framework references.
  -->
  <ItemGroup>
-    <FrameworkReference Remove="Microsoft.AspNetCore.App" />
+    <FrameworkReference Remove="Microsoft.AspNetCore.App" Condition="'$(DoNotApplyWorkaroundsToMicrosoftAspNetCoreApp)' != 'true'" />


FYI @wtgodbe

pranavkm · 2020-08-05T17:50:19Z

Directory.Build.targets

@@ -147,7 +147,7 @@
    <KnownFrameworkReference Condition="'$(UseAspNetCoreSharedRuntime)' != 'true'" Remove="Microsoft.AspNetCore.App" />
    <KnownFrameworkReference Remove="Microsoft.WindowsDesktop.App" />

-    <KnownFrameworkReference Condition="'$(UseAspNetCoreSharedRuntime)' == 'true'" Update="Microsoft.AspNetCore.App">
+    <KnownFrameworkReference Condition="'$(UseAspNetCoreSharedRuntime)' == 'true' AND '$(DoNotApplyWorkaroundsToMicrosoftAspNetCoreApp)' != 'true'" Update="Microsoft.AspNetCore.App">


FYI @wtgodbe

Remind me what the motivation was for this?

We wanted a middleware that targeted netcoreapp3.1 and compiled against M.AspNetCore.App. UseAspNetCoreSharedRuntime is required to keep the shared runtime reference around, but I didn't want to compile against 5.0.0-dev.

jkotalik · 2020-08-05T18:00:02Z

src/Tools/dotnet-watch/BrowserRefresh/src/StartupHook.cs

+{
+    public static void Initialize()
+    {
+        // We're in!


Maybe a bit more details here 😄

src/Tools/dotnet-watch/BrowserRefresh/src/StartupHook.cs

src/Tools/dotnet-watch/BrowserRefresh/src/BrowserRefreshMiddleware.cs

src/Tools/dotnet-watch/BrowserRefresh/src/ResponseStreamWrapper.cs

src/Tools/dotnet-watch/BrowserRefresh/src/WebSocketScriptInjection.cs

src/Tools/dotnet-watch/BrowserRefresh/test/WebSockerScriptInjectionTest.cs

src/Tools/dotnet-watch/BrowserRefresh/src/WebSocketScriptInjection.cs

Tratcher · 2020-08-05T20:54:28Z

src/Tools/dotnet-watch/BrowserRefresh/src/WebSocketScriptInjection.cs

+
+        private static string GetWebSocketClientJavaScript()
+        {
+            var hostString = Environment.GetEnvironmentVariable("ASPNETCORE_AUTO_RELOAD_WS_ENDPOINT");


Should this really be pulling direct from env variables vs reading through config and/or options?

The middleware doesn't really interact with the remainder of the user app so an option definitely seems overkill. There isn't really a scenario where we'd want to use values that dotnet-watch did not set. Do you think it's still useful for us to use config here?

At a minimum some indirection would make it easier to test.

ghost · 2020-08-05T22:35:05Z

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

pranavkm · 2020-08-05T22:50:29Z

🆙 📅

davidfowl · 2020-08-05T23:12:09Z

src/Tools/dotnet-watch/BrowserRefresh/src/BrowserRefreshMiddleware.cs

+        public BrowserRefreshMiddleware(RequestDelegate next, ILogger<BrowserRefreshMiddleware> logger) =>
+            (_next, _logger) = (next, logger);
+
+        public async Task InvokeAsync(HttpContext context)


How do I disable this?

There are environment switches for dotnet-watch to turn this off:

DOTNET_WATCH_SUPPRESS_BROWSER_REFRESH=0. I should update the documentation for this.

davidfowl · 2020-08-05T23:12:56Z

src/Tools/dotnet-watch/BrowserRefresh/src/BrowserRefreshMiddleware.cs

+                return false;
+            }
+
+            var typedHeaders = request.GetTypedHeaders();


Can you do a fast path check for a text\html content type

src/Tools/dotnet-watch/BrowserRefresh/src/WebSocketScriptInjection.cs

Tratcher · 2020-08-05T23:15:17Z

src/Tools/dotnet-watch/BrowserRefresh/src/ResponseStreamWrapper.cs

+
+            if (IsHtmlResponse && !ScriptInjectionPerformed)
+            {
+                ScriptInjectionPerformed = await WebSocketScriptInjection.Instance.TryInjectLiveReloadScriptAsync(_baseStream, buffer, offset, count);


Suggested change

ScriptInjectionPerformed = await WebSocketScriptInjection.Instance.TryInjectLiveReloadScriptAsync(_baseStream, buffer, offset, count);

ScriptInjectionPerformed = await WebSocketScriptInjection.Instance.TryInjectLiveReloadScriptAsync(_baseStream, buffer, offset, count, cancellationToken);

Tratcher · 2020-08-05T23:15:51Z

src/Tools/dotnet-watch/BrowserRefresh/src/ResponseStreamWrapper.cs

+                    _baseStream,
+                    materialized,
+                    0,
+                    materialized.Length);


Suggested change

materialized.Length);

materialized.Length,

cancellationToken);

pranavkm · 2020-08-05T23:17:16Z

Description

As of 5.0-preview8, dotnet-watch run supports launching the browser as part of starting an application. This change further enhances the experience by refreshing the browser on file changes.

Customer impact

Changes to dotnet-watch come from our CLI user testing. We think these are compelling scenarios we'd like to get early feedback for.

Regression

No

Risk

Medium. This change includes injecting code in to a user's app launched via dotnet watch run and rewriting the output. There's a chance that we break the user output, however we'd like to find these out sooner so we can address it for rc1 and there are switches to turn off this behavior if it affects developers.

Tratcher · 2020-08-05T23:55:01Z

...TestProjects/KitchenSink/.net/objDebug/net5.0/.NETCoreApp,Version=v5.0.AssemblyAttributes.cs

+// <autogenerated />
+using System;
+using System.Reflection;
+[assembly: global::System.Runtime.Versioning.TargetFrameworkAttribute(".NETCoreApp,Version=v5.0", FrameworkDisplayName = "")]


These shouldn't be checked in should they?

Nope. I should get to the bottom of why they get generated in these path.

dougbu

First comment is the most important

src/Tools/dotnet-watch/BrowserRefresh/src/Microsoft.AspNetCore.Watch.BrowserRefresh.csproj

dougbu · 2020-08-05T23:52:29Z

src/Tools/dotnet-watch/src/dotnet-watch.csproj

+      PrivateAssets="All"
+      ReferenceOutputAssembly="false"
+      SkipGetTargetFrameworkProperties="true"
+      UndefineProperties="TargetFramework;TargetFrameworks" />


Not now (because the metadata won't be handled correctly) but when this is merging into 'master', switch to use a @(Reference) item instead. We're avoiding direct project references as much as possible.

dougbu · 2020-08-05T23:56:29Z

src/Tools/dotnet-watch/src/dotnet-watch.csproj

  <Target Name="_FixupRuntimeConfig" BeforeTargets="_GenerateRuntimeConfigurationFilesInputCache">
    <ItemGroup>
      <_RuntimeFramework Include="@(RuntimeFramework)" />
      <RuntimeFramework Remove="@(RuntimeFramework)" />
-      <RuntimeFramework Include="Microsoft.AspNetCore.App" FrameworkName="Microsoft.AspNetCore.App" Version="5.0.0-preview" />
+      <RuntimeFramework Include="Microsoft.AspNetCore.App" FrameworkName="Microsoft.AspNetCore.App" Version="5.0.0-preview.7.20329.4" />


Why this specific version instead of letting it float? This looks odd outside eng/Versions.props in any case.

I changed this back to the earlier version. I had an earlier build of the runtime locally which didn't have some of the newer runtime types such as the non-generic TaskCompletionSource. I need to sort this out soon for rc1 since it also affects blazor

...TestProjects/KitchenSink/.net/objDebug/net5.0/.NETCoreApp,Version=v5.0.AssemblyAttributes.cs

pranavkm force-pushed the prkrishn/browser-refresh branch 3 times, most recently from b401d73 to 004d735 Compare August 5, 2020 16:50

pranavkm marked this pull request as ready for review August 5, 2020 17:21

pranavkm requested review from BrennanConroy, Tratcher and wtgodbe August 5, 2020 17:21

Add a middleware for browser refresh.

54ad088

* Introduce a middleware that can connect to the dotnet-watch change server * dotnet-watch: Inject the middleware in 3.1 or apps using start hooks \ hosting startup #23412

pranavkm force-pushed the prkrishn/browser-refresh branch from 004d735 to 54ad088 Compare August 5, 2020 17:40

pranavkm commented Aug 5, 2020

View reviewed changes

jkotalik reviewed Aug 5, 2020

View reviewed changes

Pilchie added the area-commandlinetools Includes: Command line tools, dotnet-dev-certs, dotnet-user-jwts, and OpenAPI label Aug 5, 2020

pranavkm commented Aug 5, 2020

View reviewed changes

src/Tools/dotnet-watch/BrowserRefresh/src/StartupHook.cs Outdated Show resolved Hide resolved

Update src/Tools/dotnet-watch/BrowserRefresh/src/StartupHook.cs

3fa42c9

Tratcher requested changes Aug 5, 2020

View reviewed changes

mkArtakMSFT added ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. Servicing-consider Shiproom approval is required for the issue labels Aug 5, 2020

mkArtakMSFT added this to the 5.0.0-preview8 milestone Aug 5, 2020

Changes per PR comments

4ecde8d

Add a test for reading the script

49e7d59

davidfowl reviewed Aug 5, 2020

View reviewed changes

src/Tools/dotnet-watch/BrowserRefresh/src/WebSocketScriptInjection.cs Outdated Show resolved Hide resolved

davidfowl reviewed Aug 5, 2020

View reviewed changes

src/Tools/dotnet-watch/BrowserRefresh/src/WebSocketScriptInjection.cs Outdated Show resolved Hide resolved

Tratcher approved these changes Aug 5, 2020

View reviewed changes

Changes per PR comments

989ecd1

Tratcher reviewed Aug 5, 2020

View reviewed changes

dougbu requested changes Aug 5, 2020

View reviewed changes

Updates docs

fa40263

pranavkm force-pushed the prkrishn/browser-refresh branch from cfae514 to fa40263 Compare August 6, 2020 00:18

mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Aug 6, 2020

Fixup test

e949291

danroth27 mentioned this pull request Aug 6, 2020

Hot Reload for Blazor #5456

Closed

4 tasks

Add project ref

5bf6cff

dougbu approved these changes Aug 6, 2020

View reviewed changes

mkArtakMSFT merged commit 146d316 into release/5.0-preview8 Aug 7, 2020

mkArtakMSFT deleted the prkrishn/browser-refresh branch August 7, 2020 00:20

rogihee mentioned this pull request Nov 6, 2020

Ideas for improvement of dev inner loop with scoped CSS development and dotnet watch run #27555

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a middleware for browser refresh. #24574

Add a middleware for browser refresh. #24574

pranavkm commented Aug 5, 2020

BrennanConroy commented Aug 5, 2020

pranavkm Aug 5, 2020

pranavkm Aug 5, 2020

wtgodbe Aug 5, 2020

pranavkm Aug 5, 2020

jkotalik Aug 5, 2020

Tratcher Aug 5, 2020

pranavkm Aug 5, 2020

Tratcher Aug 5, 2020

ghost commented Aug 5, 2020

pranavkm commented Aug 5, 2020

davidfowl Aug 5, 2020

pranavkm Aug 5, 2020

davidfowl Aug 5, 2020

Tratcher Aug 5, 2020

Tratcher Aug 5, 2020

pranavkm commented Aug 5, 2020

Tratcher Aug 5, 2020

pranavkm Aug 6, 2020

dougbu left a comment

dougbu Aug 5, 2020

dougbu Aug 5, 2020

pranavkm Aug 6, 2020

	ScriptInjectionPerformed = await WebSocketScriptInjection.Instance.TryInjectLiveReloadScriptAsync(_baseStream, buffer, offset, count);
	ScriptInjectionPerformed = await WebSocketScriptInjection.Instance.TryInjectLiveReloadScriptAsync(_baseStream, buffer, offset, count, cancellationToken);

	materialized.Length);
	materialized.Length,
	cancellationToken);

Add a middleware for browser refresh. #24574

Add a middleware for browser refresh. #24574

Conversation

pranavkm commented Aug 5, 2020

BrennanConroy commented Aug 5, 2020

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

ghost commented Aug 5, 2020

pranavkm commented Aug 5, 2020

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

pranavkm commented Aug 5, 2020

Description

Customer impact

Regression

Risk

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu 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